On Tue, Oct 8, 2024 at 6:13 PM Steven Rostedt <[email protected]> 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
