Re: [PATCH] tracing: replace multiple deprecated strncpy with strscpy

[Justin Stitt]

On Tue, Oct 8, 2024 at 6:13 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> So this breaks my tests. This is why I have trouble with taking changes
> like this :-(

Shoot.

I think deprecated API cleanups are important but creating more bugs
in the process is not helping anybody. I dropped the ball here... my
patch has an off-by-one.

>
> Before this patch, his worked:
>
>  # echo 'common_pid != 0 && common_pid != 120 && common_pid != 1253 && common_pid != 17 && common_pid != 394 && common_pid != 81 && common_pid != 87' > /sys/kernel/tracing/events/sched/sched_switch/filter

Thanks for providing the test case, this made triaging dead simple.

>
> But now it gives an error of:
>
>  -bash: echo: write error: Invalid argument
>
> I have to drop this.

In many cases where folks are doing 1) strncpy and 2) manual NUL-byte
assignment, the clear replacement is strscpy. However most of those
cases look like this:

  strncpy(dst, src, len);
  dst[len-1] = '\0';

and this case was just

  strncpy(dst, src, len);
  dst[len] = '\0';

Since we have an explicit size check before the first copy, ensuring
@len doesn't overflow @dst, this code is fine but I missed the
off-by-one.

So, assuming I haven't lost your faith, I can send a v2 along the lines of:

1)
  strscpy(num_buf, str + s, len + 1);

  ... or
2)
  memcpy(num_buf, str + s, len);
  num_buf[len] = 0;

And if you're wondering about option 3: "Don't change anything because
the code works". I'd reiterate that I think it's important to replace
bad ambiguous APIs. There are many cases where folks use strncpy() as
a glorified memcpy because they want the padding behavior, or they use
it on non-null terminated destinations or tons of other "misuses".
Ambiguous code like that poses a real danger to the maintainability of
the codebase and opens threat vectors.

>
> -- Steve

Thanks
Justin