On Thu, 4 Nov 2021 19:28:49 +0200
Tzvetomir Stoyanov <tz.stoyanov@xxxxxxxxx> wrote:
> On Thu, Nov 4, 2021 at 6:33 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> >
> > On Thu, 4 Nov 2021 13:10:41 +0200
> > "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@xxxxxxxxx> wrote:
> >
> > > +enum tracefs_dynevent_type tracefs_kprobe_info(struct tracefs_dynevent *kprobe,
> > > + char **system, char **event,
> > > + char **prefix, char **addr, char **format)
> > > +{
> > > + char **lv[] = { system, event, prefix, addr, format };
> > > + char **rv[] = { &kprobe->system, &kprobe->event, &kprobe->prefix,
> > > + &kprobe->address, &kprobe->format };
> > > + int i;
> > > +
> > > + if (!kprobe)
> > > + return TRACEFS_DYNEVENT_MAX;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(lv); i++)
> > > + *lv[i] = NULL;
> >
> > Do we really need to initialize them to NULL here?
> >
> > Not to mention, if one of the parameters is NULL itself, this will SEGFAULT.
> >
>
> I'll add a check for that. They should be set to NULL to ensure that
> free() in the error path will not segfault, if the caller passes not
> initialised pointers.
It wont, due to the fact that it starts at i and works backward down to
zero. The only items it will look at are the ones that were already
initialized. Which is why I did it that way.
>
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(lv); i++) {
> >
> > Nice use of ARRAY_SIZE() ;-)
> >
> > > + if (lv[i]) {
> > > + if (*rv[i]) {
> > > + *lv[i] = strdup(*rv[i]);
> > > + if (!*lv[i])
> > > + goto error;
> > > + } else {
> > > + *lv[i] = NULL;
> > > }
> > > +
> > > + return kprobe->type;
> > > +
> > > +error:
> > > + for (i--; i >= 0; i--) {
The above means that we do not need to initialize anything that wasn't
touched.
-- Steve
> > > + if (lv[i])
> > > + free(*lv[i]);
> > > + }
> > > +
> > > + return TRACEFS_DYNEVENT_MAX;
> > > }
> >
>
>
|