| Message ID | 20180323010729.41457-1-raj.khem@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Replace strncpy with memccpy to fix -Wstringop-truncation. | expand |
On Thu, 22 Mar 2018, Khem Raj wrote: > * nis/nss_nisplus/nisplus-parser.c: Replace strncpy with memcpy to > avoid -Wstringop-truncation. Could you give more details of (a) in what circumstances (architecture, compiler, etc.) you get this warning and (b) why the truncation is correct? Whatever build failure you got hasn't shown up with build-many-glibcs.py. Maybe that should include a configuration with --enable-obsolete-rpc --enable-obsolete-nsl to make sure that it tests building those bits of code that are disabled by default? -- Joseph S. Myers joseph@codesourcery.com
Hi Joseph On Thu, Mar 22, 2018 at 6:17 PM, Joseph Myers <joseph@codesourcery.com> wrote: > On Thu, 22 Mar 2018, Khem Raj wrote: > >> * nis/nss_nisplus/nisplus-parser.c: Replace strncpy with memcpy to >> avoid -Wstringop-truncation. > > Could you give more details of (a) in what circumstances (architecture, > compiler, etc.) you get this warning and (b) why the truncation is > correct? > > Whatever build failure you got hasn't shown up with build-many-glibcs.py. > Maybe that should include a configuration with --enable-obsolete-rpc > --enable-obsolete-nsl to make sure that it tests building those bits of > code that are disabled by default? > This is seen with gcc/trunk when cross compiling for armv7ve target and yes --enable-obsolete-rpc is used to configure, I am using OpenEmbedded build system. nss_nisplus/nisplus-parser.c:90:7: error: 'strncpy' destination unchanged after copying no bytes [-Werror=stringop-truncation] strncpy (first_unused, numstr, len); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ nss_nisplus/nisplus-parser.c:106:7: error: 'strncpy' destination unchanged after copying no bytes [-Werror=stringop-truncation] strncpy (first_unused, numstr, len); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
On 03/23/2018 02:29 AM, Khem Raj wrote: > Hi Joseph > > On Thu, Mar 22, 2018 at 6:17 PM, Joseph Myers <joseph@codesourcery.com> wrote: >> On Thu, 22 Mar 2018, Khem Raj wrote: >> >>> * nis/nss_nisplus/nisplus-parser.c: Replace strncpy with memcpy to >>> avoid -Wstringop-truncation. >> >> Could you give more details of (a) in what circumstances (architecture, >> compiler, etc.) you get this warning and (b) why the truncation is >> correct? >> >> Whatever build failure you got hasn't shown up with build-many-glibcs.py. >> Maybe that should include a configuration with --enable-obsolete-rpc >> --enable-obsolete-nsl to make sure that it tests building those bits of >> code that are disabled by default? >> > > This is seen with gcc/trunk when cross compiling for armv7ve target > and yes --enable-obsolete-rpc is used to configure, I am using > OpenEmbedded build system. > > nss_nisplus/nisplus-parser.c:90:7: error: 'strncpy' destination > unchanged after copying no bytes [-Werror=stringop-truncation] > strncpy (first_unused, numstr, len); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > nss_nisplus/nisplus-parser.c:106:7: error: 'strncpy' destination > unchanged after copying no bytes [-Werror=stringop-truncation] > strncpy (first_unused, numstr, len); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > Hi, I also see those warnings on s390x if build with gcc-head (from today) and if glibc is configured with --enable-obsolete-nsl. Why do we need the strncpy at all? if (len == 0 && ...) { ... strncpy (first_unused, numstr, len); first_unused[len] = '\0'; ... } Bye. Stefan
On Apr 05 2018, Stefan Liebler <stli@linux.vnet.ibm.com> wrote: > Why do we need the strncpy at all? > if (len == 0 && ...) That's obviously a typo. Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
On 04/05/2018 06:14 PM, Andreas Schwab wrote: > On Apr 05 2018, Stefan Liebler <stli@linux.vnet.ibm.com> wrote: > >> Why do we need the strncpy at all? >> if (len == 0 && ...) > > That's obviously a typo. > > Andreas. > Yes. You are right. Please have a look at the applied patch. If the zero-termination is needed, numstr is copied to the buffer with strncpy and the zero-termination is applied. If numstr is either 0 bytes long or the length of the numstr string is 0, then _nss_nisplus_parse_pwent returns with 0. This solves the mentioned warning with if build with gcc-head and --enable-obsolete-nsl. But I can not test it as I don't have a nisplus setup. Bye. Stefan diff --git a/nis/nss_nisplus/nisplus-parser.c b/nis/nss_nisplus/nisplus-parser.c index 8dc021e73d..4714a3085a 100644 --- a/nis/nss_nisplus/nisplus-parser.c +++ b/nis/nss_nisplus/nisplus-parser.c @@ -82,7 +82,7 @@ _nss_nisplus_parse_pwent (nis_result *result, struct passwd *pw, char *numstr = NISOBJVAL (2, obj); len = NISOBJLEN (2, obj); - if (len == 0 && numstr[len - 1] != '\0') + if (len != 0 && numstr[len - 1] != '\0') { if (len >= room_left) goto no_more_room; @@ -91,14 +91,14 @@ _nss_nisplus_parse_pwent (nis_result *result, struct passwd *pw, first_unused[len] = '\0'; numstr = first_unused; } - if (numstr[0] == '\0') + if (len == 0 || numstr[0] == '\0') /* If we don't have a uid, it's an invalid shadow entry. */ return 0; pw->pw_uid = strtoul (numstr, NULL, 10); numstr = NISOBJVAL (3, obj); len = NISOBJLEN (3, obj); - if (len == 0 && numstr[len - 1] != '\0') + if (len != 0 && numstr[len - 1] != '\0') { if (len >= room_left) goto no_more_room; @@ -107,7 +107,7 @@ _nss_nisplus_parse_pwent (nis_result *result, struct passwd *pw, first_unused[len] = '\0'; numstr = first_unused; } - if (numstr[0] == '\0') + if (len == 0 || numstr[0] == '\0') /* If we don't have a gid, it's an invalid shadow entry. */ return 0; pw->pw_gid = strtoul (numstr, NULL, 10);
On 04/12/2018 05:31 PM, Stefan Liebler wrote: > On 04/05/2018 06:14 PM, Andreas Schwab wrote: >> On Apr 05 2018, Stefan Liebler <stli@linux.vnet.ibm.com> wrote: >> >>> Why do we need the strncpy at all? >>> if (len == 0 && ...) >> >> That's obviously a typo. >> >> Andreas. >> > Yes. You are right. Please have a look at the applied patch. > If the zero-termination is needed, numstr is copied to the buffer with > strncpy and the zero-termination is applied. > If numstr is either 0 bytes long or the length of the numstr string is > 0, then _nss_nisplus_parse_pwent returns with 0. > > > This solves the mentioned warning with if build with gcc-head and > --enable-obsolete-nsl. > But I can not test it as I don't have a nisplus setup. > > Bye. > Stefan PING
diff --git a/nis/nss_nisplus/nisplus-parser.c b/nis/nss_nisplus/nisplus-parser.c index 8dc021e73d..b53284f889 100644 --- a/nis/nss_nisplus/nisplus-parser.c +++ b/nis/nss_nisplus/nisplus-parser.c @@ -87,7 +87,7 @@ _nss_nisplus_parse_pwent (nis_result *result, struct passwd *pw, if (len >= room_left) goto no_more_room; - strncpy (first_unused, numstr, len); + memcpy (first_unused, numstr, len); first_unused[len] = '\0'; numstr = first_unused; } @@ -103,7 +103,7 @@ _nss_nisplus_parse_pwent (nis_result *result, struct passwd *pw, if (len >= room_left) goto no_more_room; - strncpy (first_unused, numstr, len); + memcpy (first_unused, numstr, len); first_unused[len] = '\0'; numstr = first_unused; }
* nis/nss_nisplus/nisplus-parser.c: Replace strncpy with memcpy to avoid -Wstringop-truncation. --- 2018-03-22 Khem Raj <raj.khem@gmail.com> * nis/nss_nisplus/nisplus-parser.c (_nss_nisplus_parse_pwent): Replace strncpy with memcpy to avoid -Wstringop-truncation. nis/nss_nisplus/nisplus-parser.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.16.2