Re: [PATCH v4 04/10] libtracefs: Change tracefs_kprobe_info API

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 4 Nov 2021 19:28:49 +0200
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:

> On Thu, Nov 4, 2021 at 6:33 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Thu,  4 Nov 2021 13:10:41 +0200
> > "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> 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;
> > >  }  
> >  
> 
>