Re: [bug report] tracing: Allow whitespace to surround hist trigger filter

["Zanussi, Tom" <tom.zanussi@linux.intel.com>]

Hi Dan,

On 1/18/2022 3:38 AM, Dan Carpenter wrote:
> [ This is an older warning but renaming the function made it appear in
>    the new warnings list.  I have searched my outbox and I don't think
>    I forwarded this one before.  -dan ]
> 
> Hello Tom Zanussi,
> 
> The patch ec5ce0987541: "tracing: Allow whitespace to surround hist
> trigger filter" from Jan 15, 2018, leads to the following Smatch
> static checker warning:
> 
> 	kernel/trace/trace_events_hist.c:6199 event_hist_trigger_parse()
> 	warn: 'p' can't be NULL.
> 
> kernel/trace/trace_events_hist.c
>      6149 static int event_hist_trigger_parse(struct event_command *cmd_ops,
>      6150                                     struct trace_event_file *file,
>      6151                                     char *glob, char *cmd, char *param)
>      6152 {
>      6153         unsigned int hist_trigger_bits = TRACING_MAP_BITS_DEFAULT;
>      6154         struct event_trigger_data *trigger_data;
>      6155         struct hist_trigger_attrs *attrs;
>      6156         struct event_trigger_ops *trigger_ops;
>      6157         struct hist_trigger_data *hist_data;
>      6158         struct synth_event *se;
>      6159         const char *se_name;
>      6160         bool remove = false;
>      6161         char *trigger, *p, *start;
>      6162         int ret = 0;
>      6163
>      6164         lockdep_assert_held(&event_mutex);
>      6165
>      6166         if (glob && strlen(glob)) {
>      6167                 hist_err_clear();
>      6168                 last_cmd_set(file, param);
>      6169         }
>      6170
>      6171         if (!param)
>      6172                 return -EINVAL;
>      6173
>      6174         if (glob[0] == '!')
>      6175                 remove = true;
>      6176
>      6177         /*
>      6178          * separate the trigger from the filter (k:v [if filter])
>      6179          * allowing for whitespace in the trigger
>      6180          */
>      6181         p = trigger = param;
>      6182         do {
>      6183                 p = strstr(p, "if");
>      6184                 if (!p)
>      6185                         break;
>      6186                 if (p == param)
>      6187                         return -EINVAL;
>      6188                 if (*(p - 1) != ' ' && *(p - 1) != '\t') {
>      6189                         p++;
>      6190                         continue;
>                                   ^^^^^^^^^
> 
> These are the continue paths
> 
>      6191                 }
>      6192                 if (p >= param + strlen(param) - (sizeof("if") - 1) - 1)
>      6193                         return -EINVAL;
>      6194                 if (*(p + sizeof("if") - 1) != ' ' && *(p + sizeof("if") - 1) != '\t') {
>      6195                         p++;
>      6196                         continue;
>                                   ^^^^^^^^^
> 
>      6197                 }
>      6198                 break;
> --> 6199         } while (p);
> 
> Should this be } while(*p);?

No, it shouldn't be *p, it should be p, but the check at the top of the loop (if (!p) break) makes it redundant.  I'll submit a patch to change it to while (1) to avoid the warning.

Thanks,

Tom

> 
>      6200
>      6201         if (!p)
>      6202                 param = NULL;
>      6203         else {
>      6204                 *(p - 1) = '\0';
>      6205                 param = strstrip(p);
>      6206                 trigger = strstrip(trigger);
>      6207         }
>      6208
> 
> regards,
> dan carpenter