| Message ID | 20220519073301.7072-1-krzysztof.kozlowski@linaro.org |
|---|---|
| State | Accepted |
| Commit | ac0126a0173531d91d164e244ed1ebbee64bcd54 |
| Headers | show |
| Series | [v2,1/2] soc: qcom: cmd-db: replace strncpy() with strscpy_pad() | expand |
On 19/05/2022 09:33, Krzysztof Kozlowski wrote: > The use of strncpy() is considered deprecated for NUL-terminated > strings[1]. Replace strncpy() with strscpy_pad(), to keep existing > pad-behavior of strncpy. This fixes W=1 warning: > > drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_get_header.part.0’: > drivers/soc/qcom/cmd-db.c:151:9: warning: ‘strncpy’ specified bound 8 equals destination size [-Wstringop-truncation] > 151 | strncpy(query, id, sizeof(query)); > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Reviewed-by: Stephen Boyd <sboyd@kernel.org> Any comments on the set? Best regards, Krzysztof
On Thu, 19 May 2022 09:33:00 +0200, Krzysztof Kozlowski wrote: > The use of strncpy() is considered deprecated for NUL-terminated > strings[1]. Replace strncpy() with strscpy_pad(), to keep existing > pad-behavior of strncpy. This fixes W=1 warning: > > drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_get_header.part.0’: > drivers/soc/qcom/cmd-db.c:151:9: warning: ‘strncpy’ specified bound 8 equals destination size [-Wstringop-truncation] > 151 | strncpy(query, id, sizeof(query)); > > [...] Applied, thanks! [1/2] soc: qcom: cmd-db: replace strncpy() with strscpy_pad() commit: ac0126a0173531d91d164e244ed1ebbee64bcd54 [2/2] soc: qcom: correct kerneldoc commit: d11a34a404ee5565ce8e0abe3e2b9ce6f5cc0a4b Best regards,
Hi, On Thu, May 19, 2022 at 12:33 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > The use of strncpy() is considered deprecated for NUL-terminated > strings[1]. Replace strncpy() with strscpy_pad(), to keep existing > pad-behavior of strncpy. This fixes W=1 warning: > > drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_get_header.part.0’: > drivers/soc/qcom/cmd-db.c:151:9: warning: ‘strncpy’ specified bound 8 equals destination size [-Wstringop-truncation] > 151 | strncpy(query, id, sizeof(query)); > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Reviewed-by: Stephen Boyd <sboyd@kernel.org> > > --- > > Changes since v1: > 1. Split series per subsystem. > 2. Add tag. > --- > drivers/soc/qcom/cmd-db.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c > index dd872017f345..c5137c25d819 100644 > --- a/drivers/soc/qcom/cmd-db.c > +++ b/drivers/soc/qcom/cmd-db.c > @@ -148,7 +148,7 @@ static int cmd_db_get_header(const char *id, const struct entry_header **eh, > return ret; > > /* Pad out query string to same length as in DB */ > - strncpy(query, id, sizeof(query)); > + strscpy_pad(query, id, sizeof(query)); Sorry to report that this breaks booting on sc7280-herobrine-herobrine-r1. I believe that the function was actually _relying_ on the "unsafe" behavior of strncpy(). Specifically I think: * The source string (id) was a '\0' terminated string. * The destination (query) was a fixed 8-byte string and doesn't need to be '\0' terminated. So before your change we could actually support 8-byte strings. Now we can't because you'll only copy 7 bytes to the destination to leave room for the '\0' termination... Looking at printouts, I see, for instance at least one ID that looks like "lnbclka2". Given the requirements of this function(), the old strncpy() is actually _exactly_ what we want. Is there any way to disable the warning? If not, I guess we could make "query" be 9 bytes bit, or "sizeof(ent->id) + 1" bytes big... Happy to post a patch, but it's basically a bikeshed for how you want it fixed (there are dozens of ways) and I'd rather you just tell me instead of me guessing. -Doug
Hi, On Mon, Jun 27, 2022 at 4:06 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Thu, May 19, 2022 at 12:33 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > > > > The use of strncpy() is considered deprecated for NUL-terminated > > strings[1]. Replace strncpy() with strscpy_pad(), to keep existing > > pad-behavior of strncpy. This fixes W=1 warning: > > > > drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_get_header.part.0’: > > drivers/soc/qcom/cmd-db.c:151:9: warning: ‘strncpy’ specified bound 8 equals destination size [-Wstringop-truncation] > > 151 | strncpy(query, id, sizeof(query)); > > > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > > > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Reviewed-by: Stephen Boyd <sboyd@kernel.org> > > > > --- > > > > Changes since v1: > > 1. Split series per subsystem. > > 2. Add tag. > > --- > > drivers/soc/qcom/cmd-db.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c > > index dd872017f345..c5137c25d819 100644 > > --- a/drivers/soc/qcom/cmd-db.c > > +++ b/drivers/soc/qcom/cmd-db.c > > @@ -148,7 +148,7 @@ static int cmd_db_get_header(const char *id, const struct entry_header **eh, > > return ret; > > > > /* Pad out query string to same length as in DB */ > > - strncpy(query, id, sizeof(query)); > > + strscpy_pad(query, id, sizeof(query)); > > Sorry to report that this breaks booting on > sc7280-herobrine-herobrine-r1. I believe that the function was > actually _relying_ on the "unsafe" behavior of strncpy(). Specifically > I think: > * The source string (id) was a '\0' terminated string. > * The destination (query) was a fixed 8-byte string and doesn't need > to be '\0' terminated. > > So before your change we could actually support 8-byte strings. Now we > can't because you'll only copy 7 bytes to the destination to leave > room for the '\0' termination... > > Looking at printouts, I see, for instance at least one ID that looks > like "lnbclka2". > > Given the requirements of this function(), the old strncpy() is > actually _exactly_ what we want. Is there any way to disable the > warning? If not, I guess we could make "query" be 9 bytes bit, or > "sizeof(ent->id) + 1" bytes big... Happy to post a patch, but it's > basically a bikeshed for how you want it fixed (there are dozens of > ways) and I'd rather you just tell me instead of me guessing. Actually, I followed the link on the error message and found that the right solution is probably '__nonstring' so I've posted that. If someone wants the bikeshed a different color I can do it a different way. https://lore.kernel.org/r/20220627161642.1.Ie7b480cd99e2c13319220cbc108caf2bcd41286b@changeid
On 28/06/2022 01:06, Doug Anderson wrote: > Hi, > > On Thu, May 19, 2022 at 12:33 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> The use of strncpy() is considered deprecated for NUL-terminated >> strings[1]. Replace strncpy() with strscpy_pad(), to keep existing >> pad-behavior of strncpy. This fixes W=1 warning: >> >> drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_get_header.part.0’: >> drivers/soc/qcom/cmd-db.c:151:9: warning: ‘strncpy’ specified bound 8 equals destination size [-Wstringop-truncation] >> 151 | strncpy(query, id, sizeof(query)); >> >> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings >> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Reviewed-by: Stephen Boyd <sboyd@kernel.org> >> >> --- >> >> Changes since v1: >> 1. Split series per subsystem. >> 2. Add tag. >> --- >> drivers/soc/qcom/cmd-db.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c >> index dd872017f345..c5137c25d819 100644 >> --- a/drivers/soc/qcom/cmd-db.c >> +++ b/drivers/soc/qcom/cmd-db.c >> @@ -148,7 +148,7 @@ static int cmd_db_get_header(const char *id, const struct entry_header **eh, >> return ret; >> >> /* Pad out query string to same length as in DB */ >> - strncpy(query, id, sizeof(query)); >> + strscpy_pad(query, id, sizeof(query)); > > Sorry to report that this breaks booting on > sc7280-herobrine-herobrine-r1. Sorry to break booting and thanks for the report. > I believe that the function was > actually _relying_ on the "unsafe" behavior of strncpy(). Specifically > I think: > * The source string (id) was a '\0' terminated string. > * The destination (query) was a fixed 8-byte string and doesn't need > to be '\0' terminated. > > So before your change we could actually support 8-byte strings. Now we > can't because you'll only copy 7 bytes to the destination to leave > room for the '\0' termination... > > Looking at printouts, I see, for instance at least one ID that looks > like "lnbclka2". Ah, crap... I did not expect that. > > Given the requirements of this function(), the old strncpy() is > actually _exactly_ what we want. Is there any way to disable the > warning? If not, I guess we could make "query" be 9 bytes bit, or > "sizeof(ent->id) + 1" bytes big... Happy to post a patch, but it's > basically a bikeshed for how you want it fixed (there are dozens of > ways) and I'd rather you just tell me instead of me guessing. If the source was not a string, I would propose memcpy(). Since it is a string, I would prefer to have safe copy, so increased size. However I see you sent now patches, so let me actually respond there. Thanks for the analysis, much appreciated! Best regards, Krzysztof
diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c index dd872017f345..c5137c25d819 100644 --- a/drivers/soc/qcom/cmd-db.c +++ b/drivers/soc/qcom/cmd-db.c @@ -148,7 +148,7 @@ static int cmd_db_get_header(const char *id, const struct entry_header **eh, return ret; /* Pad out query string to same length as in DB */ - strncpy(query, id, sizeof(query)); + strscpy_pad(query, id, sizeof(query)); for (i = 0; i < MAX_SLV_ID; i++) { rsc_hdr = &cmd_db_header->header[i];
The use of strncpy() is considered deprecated for NUL-terminated strings[1]. Replace strncpy() with strscpy_pad(), to keep existing pad-behavior of strncpy. This fixes W=1 warning: drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_get_header.part.0’: drivers/soc/qcom/cmd-db.c:151:9: warning: ‘strncpy’ specified bound 8 equals destination size [-Wstringop-truncation] 151 | strncpy(query, id, sizeof(query)); [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Reviewed-by: Stephen Boyd <sboyd@kernel.org> --- Changes since v1: 1. Split series per subsystem. 2. Add tag. --- drivers/soc/qcom/cmd-db.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)