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 <[email protected]>
Reviewed-by: Stephen Boyd <[email protected]>
---
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));
for (i = 0; i < MAX_SLV_ID; i++) {
rsc_hdr = &cmd_db_header->header[i];
--
2.32.0
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 <[email protected]>
> Reviewed-by: Stephen Boyd <[email protected]>
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,
--
Bjorn Andersson <[email protected]>
Hi,
On Thu, May 19, 2022 at 12:33 AM Krzysztof Kozlowski
<[email protected]> 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 <[email protected]>
> Reviewed-by: Stephen Boyd <[email protected]>
>
> ---
>
> 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 <[email protected]> wrote:
>
> Hi,
>
> On Thu, May 19, 2022 at 12:33 AM Krzysztof Kozlowski
> <[email protected]> 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 <[email protected]>
> > Reviewed-by: Stephen Boyd <[email protected]>
> >
> > ---
> >
> > 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
> <[email protected]> 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 <[email protected]>
>> Reviewed-by: Stephen Boyd <[email protected]>
>>
>> ---
>>
>> 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