Re: [PATCH] tracing: Check for NULL field_name in __synth_event_add_val()

[Steven Rostedt]

On Sun, 19 Feb 2023 15:46:24 -0600
Tom Zanussi <zanussi@xxxxxxxxxx> wrote:


> No, because this code just above it makes sure you can't mix add_name
> with add_next.  Once add_name is set it will return -EINVAL if
> field_name is ever null after that, and add_name will never be changed
> once set:
> 
>        /* can't mix add_next_synth_val() with add_synth_val() */
>         if (field_name) {
>                 if (trace_state->add_next) {
>                         ret = -EINVAL;
>                         goto out;
>                 }
>                 trace_state->add_name = true;
>         } else {
>                 if (trace_state->add_name) {
>                         ret = -EINVAL;
>                         goto out;
>                 }
>                 trace_state->add_next = true;
>         }
> 
> 
> >  kernel/trace/trace_events_synth.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/kernel/trace/trace_events_synth.c
> > b/kernel/trace/trace_events_synth.c
> > index 70bddb25d9c0..fa28c1da06d2 100644
> > --- a/kernel/trace/trace_events_synth.c
> > +++ b/kernel/trace/trace_events_synth.c
> > @@ -1982,6 +1982,10 @@ static int __synth_event_add_val(const char
> > *field_name, u64 val,
> >  
> >         event = trace_state->event;
> >         if (trace_state->add_name) {
> > +               if (!field_name) {
> > +                       ret = -EINVAL;
> > +                       goto out;
> > +               }  
> 
> So if add_name is set here, it must also mean that field_name can't be
> null, because of the above.
> 
> >                 for (i = 0; i < event->n_fields; i++) {
> >                         field = event->fields[i];
> >                         if (strcmp(field->name, field_name) == 0)  
> 
> And if field_name can't be null, then I don't see how this strcmp could
> fail due to a null field_name.
> 
> So I don't see the need for this patch.  The bugzilla shows a compiler
> warning when using -Wnonnull - could this just be a spurious gcc
> warning?

Thanks, I should have caught that (I was even looking for that logic,
but still missed it). That's what I get for writing patches while jet-lagged :-p

-- Steve