Replace strncpy with memccpy to fix -Wstringop-truncation.

Message ID 20180323010729.41457-1-raj.khem@gmail.com
State New
Headers show
Series
  • Replace strncpy with memccpy to fix -Wstringop-truncation.
Related show

Commit Message

Khem Raj March 23, 2018, 1:07 a.m.
* 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(-)

Comments

Joseph Myers March 23, 2018, 1:17 a.m. | #1
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?
Khem Raj March 23, 2018, 1:29 a.m. | #2
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);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Stefan Liebler April 5, 2018, 3:57 p.m. | #3
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
Andreas Schwab April 5, 2018, 4:14 p.m. | #4
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.
Stefan Liebler April 12, 2018, 3:31 p.m. | #5
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);
Stefan Liebler April 19, 2018, 2:30 p.m. | #6
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

Patch

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;
     }