diff mbox series

[v2] nisplus: Correct pwent parsing issue and resulting compilation error [BZ #23266]

Message ID alpine.DEB.2.00.1806181657110.20622@tp.orcam.me.uk
State Superseded
Headers show
Series [v2] nisplus: Correct pwent parsing issue and resulting compilation error [BZ #23266] | expand

Commit Message

Maciej W. Rozycki June 18, 2018, 4:12 p.m. UTC 
Copy and null-terminate NIS+ password file UID and GID entries whose 
length is non-zero and are not terminated in addition to empty ones, 
fixing a bug and a compilation issue causing an error with GCC 8:

nss_nisplus/nisplus-parser.c: In function '_nss_nisplus_parse_pwent':
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);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

introduced with commit ac05397075f6:

commit ac05397075f621cfdbe1db527c96167a58b6d18e
Author: Ulrich Drepper <drepper@redhat.com>
Date:   Sun Apr 30 07:01:26 2006 +0000

	* nis/nss_nisplus/nisplus-parser.c: Minor optimizations and
	cleanups.  Avoid copying data if it can be used in the old place.

(no mailing list reference available).  Obviously regardless of the 
recently added compiler diagnostics causing a build error this code has 
been long non-functional, so I guess NIS+ servers have been supplying 
strings that are non-empty and have already been null-terminated.  
Which in turn made it unnecessary to make a null-terminated copy, 
masking this bug.

	[BZ #23266]
	* nis/nss_nisplus/nisplus-parser.c (_nss_nisplus_parse_pwent):
	Copy and null-terminate entries that are not terminated in 
	addition to empty ones.
---
On Mon, 18 Jun 2018, Andreas Schwab wrote:

> I don't think this is correct.  If len == 0 then numstr[0] is undefined.

 Right, given that this is legacy code, let's take the path of least 
resistance and copy the approach from `_nss_nisplus_parse_grent', which is 
actually correct and makes sure that numstr[0] == '\0' if len == 0.

 OK for this version?

  Maciej
---
 nis/nss_nisplus/nisplus-parser.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

glibc-nisplus-parse-pwent-len.diff

Comments

Maciej W. Rozycki June 25, 2018, 7:17 p.m. UTC | #1 
On Mon, 18 Jun 2018, Maciej W. Rozycki wrote:

>  Right, given that this is legacy code, let's take the path of least 
> resistance and copy the approach from `_nss_nisplus_parse_grent', which is 
> actually correct and makes sure that numstr[0] == '\0' if len == 0.
> 
>  OK for this version?

 Ping for:

<https://patchwork.sourceware.org/patch/27908/>

Previous discussion:

<https://patchwork.sourceware.org/patch/27906/>

  Maciej
diff mbox series

Patch

Index: glibc/nis/nss_nisplus/nisplus-parser.c
===================================================================
--- glibc.orig/nis/nss_nisplus/nisplus-parser.c	2018-06-18 16:54:13.077894266 +0100
+++ glibc/nis/nss_nisplus/nisplus-parser.c	2018-06-18 16:55:04.831413510 +0100
@@ -82,7 +82,7 @@  _nss_nisplus_parse_pwent (nis_result *re
 
   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;
@@ -98,7 +98,7 @@  _nss_nisplus_parse_pwent (nis_result *re
 
   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;