Re: [bug report] tracing, synthetic events: Replace buggy strcat() with seq_buf operations

[Steven Rostedt]

On Mon, 2 Nov 2020 10:45:24 +0300
Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:

> Hello Steven Rostedt (VMware),
> 
> The patch 761a8c58db6b: "tracing, synthetic events: Replace buggy
> strcat() with seq_buf operations" from Oct 23, 2020, leads to the
> following static checker warning:
> 
> 	kernel/trace/trace_events_synth.c:703 parse_synth_field()
> 	warn: passing zero to 'ERR_PTR'
> 
> kernel/trace/trace_events_synth.c
>    582  static struct synth_field *parse_synth_field(int argc, const char **argv,
>    583                                               int *consumed)
>    584  {
>    585          struct synth_field *field;
>    586          const char *prefix = NULL, *field_type = argv[0], *field_name, *array;
>    587          int len, ret = 0;
>    588          struct seq_buf s;
>    589          ssize_t size;
>    590  
>    591          if (field_type[0] == ';')
>    592                  field_type++;
>    593  
>    594          if (!strcmp(field_type, "unsigned")) {
>    595                  if (argc < 3) {
>    596                          synth_err(SYNTH_ERR_INCOMPLETE_TYPE, errpos(field_type));
>    597                          return ERR_PTR(-EINVAL);
>    598                  }
>    599                  prefix = "unsigned ";
>    600                  field_type = argv[1];
>    601                  field_name = argv[2];
>    602                  *consumed = 3;
>    603          } else {
>    604                  field_name = argv[1];
>    605                  *consumed = 2;
>    606          }
>    607  
>    608          field = kzalloc(sizeof(*field), GFP_KERNEL);
>    609          if (!field)
>    610                  return ERR_PTR(-ENOMEM);
>    611  
>    612          len = strlen(field_name);
>    613          array = strchr(field_name, '[');
>    614          if (array)
>    615                  len -= strlen(array);
>    616          else if (field_name[len - 1] == ';')
>    617                  len--;
>    618  
>    619          field->name = kmemdup_nul(field_name, len, GFP_KERNEL);
>    620          if (!field->name) {
>    621                  ret = -ENOMEM;
>    622                  goto free;
>    623          }
>    624          if (!is_good_name(field->name)) {
>    625                  synth_err(SYNTH_ERR_BAD_NAME, errpos(field_name));
>    626                  ret = -EINVAL;
>    627                  goto free;
>    628          }
>    629  
>    630          if (field_type[0] == ';')
>    631                  field_type++;
>    632          len = strlen(field_type) + 1;
>    633  
>    634          if (array)
>    635                  len += strlen(array);
>    636  
>    637          if (prefix)
>    638                  len += strlen(prefix);
>    639  
>    640          field->type = kzalloc(len, GFP_KERNEL);
>    641          if (!field->type) {
>    642                  ret = -ENOMEM;
>    643                  goto free;
>    644          }
>    645          seq_buf_init(&s, field->type, len);
>    646          if (prefix)
>    647                  seq_buf_puts(&s, prefix);
>    648          seq_buf_puts(&s, field_type);
>    649          if (array) {
>    650                  seq_buf_puts(&s, array);
>    651                  if (s.buffer[s.len - 1] == ';')
>    652                          s.len--;
>    653          }
>    654          if (WARN_ON_ONCE(!seq_buf_buffer_left(&s)))
>    655                  goto free;
> 
> This was originally reported by kbuild-bot, but it was somehow
> overlooked so now it's on my system.  The missing error code will lead
> to a NULL dereference in the caller.


I misread the original report, modified it to fix something else, and
didn't see the real problem.

Having to initialize ret for *every* error path is ridiculous. Here's the
fix.

-- Steve