Messages in this thread | | | From | Justin Stitt <> | Date | Tue, 9 Apr 2024 13:48:38 -0700 | Subject | Re: [PATCH v2] kdb: replace deprecated strncpy |
| |
Hi,
On Tue, Apr 9, 2024 at 11:36 AM Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Mon, Apr 08, 2024 at 05:46:42PM -0700, Justin Stitt wrote: > > On Fri, Apr 5, 2024 at 2:51 AM Daniel Thompson > > <daniel.thompson@linaro.org> wrote: > > > > > > > len_tmp = strlen(p_tmp); > > > > - strncpy(cp, p_tmp+len, len_tmp-len + 1); > > > > + strscpy(cp, p_tmp+len, len_tmp-len + 1); > > > > > > Again, I really don't think the third argument provides the number of > > > characters in the destination buffer. > > > > > > > Right, the third argument is the length of the "remaining" characters > > from the completion point. > > Which is not how strscpy() is designed to be used. > > > > if you type "tes" and press tab then kallsyms_symbol_complete() will > > populate p_tmp with "test". Prior to rendering to the user, @cp points > > to "s", we need to catch the user up and print the rest of the symbol > > name since they've already typed "tes" we only need to print out "t". > > I'm more concerned about the case where you fill the buffer entirely > then move the cursor left until you get to the tes and then press Tab. > I think at the point we write too many bytes to cp. > > > > len_tmp is the length of the entire symbol part as returned by > > kallsyms_symbol_complete() and len is the length of only the > > user-typed symbol. Therefore, the amount of remaining characters to > > print is given by len_tmp-len (and +1 for a NUL-byte). > > > > So, yeah, you're right. This isn't the length of the destination but I > > don't see why we can't use memcpy() (or strscpy()) and have this not > > be considered "broken". The pointer arithmetic checks out. > > The problem with substituting strncpy() with memcpy() is that is *not* > obviously wrong... but it could be subtly wrong. > > We can see that the person who originally wrote this code made a pretty > serious mistake with strncpy() and the third argument if garbage. It is > therefore important to figure out what the *correct* value for argument > #3 should have been *before* we attempt to replace strncpy() with > anything. > > Transforming something we know to be broken without fixing it first > means it is impossible to know if the transformation is correct or not. > Hence the original question, how do we know there is enough space > after cp to store the string?
Gotcha, I will find time to seriously refactor/rewrite this function (or at the very least the tab handling part of it).
At the end of the day, though, I just want this strncpy() gone.
> > > Daniel.
Thanks Justin
| |