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.
> > +
> > + 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;
> > }
>
> The above here sets it to NULL if the parameter is non NULL.
>
> We don't need the initial NULL setting loop.
>
> -- Steve
>
>
> > - break;
> > }
> > - ret = parse_kprobe(NULL, &saveptr, &ktype, &system, &probe,
> > - &kaddr, &kfmt);
> > }
> > - free(content);
> > - return rtype;
> > +
> > + return kprobe->type;
> > +
> > +error:
> > + for (i--; i >= 0; i--) {
> > + if (lv[i])
> > + free(*lv[i]);
> > + }
> > +
> > + return TRACEFS_DYNEVENT_MAX;
> > }
>
--
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center
|