diff mbox

Avoid overlapping addresses to stpcpy calls in nscd (BZ #16760)

Message ID 20140327040406.GA26264@spoyarek.pnq.redhat.com
State New
Headers show

Commit Message

Siddhesh Poyarekar March 27, 2014, 4:04 a.m. UTC 
Calls to stpcpy from nscd netgroups code will have overlapping source
and destination when all three values in the returned triplet are
non-NULL and in the expected (host,user,domain) order.  This is seen
in valgrind as:

==3181== Source and destination overlap in stpcpy(0x19973b48, 0x19973b48)
==3181==    at 0x4C2F30A: stpcpy (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==3181==    by 0x12567A: addgetnetgrentX (string3.h:111)
==3181==    by 0x12722D: addgetnetgrent (netgroupcache.c:665)
==3181==    by 0x11114C: nscd_run_worker (connections.c:1338)
==3181==    by 0x4E3C102: start_thread (pthread_create.c:309)
==3181==    by 0x59B81AC: clone (clone.S:111)
==3181==

Fix this by using memmove instead of stpcpy.  Tested x86_64 using
various combinations of triplets (including NULL and non-NULL ones) to
verify that this works correctly and there are no regressions.

Siddhesh

	[BZ #16760]
	* nscd/netgroupcache.c (addgetnetgrentX): Use memmove instead
	of stpcpy.

---
 nscd/netgroupcache.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Comments

Mike Frysinger March 27, 2014, 7:34 a.m. UTC | #1 
On Thu 27 Mar 2014 09:34:06 Siddhesh Poyarekar wrote:
> Calls to stpcpy from nscd netgroups code will have overlapping source
> and destination when all three values in the returned triplet are
> non-NULL and in the expected (host,user,domain) order.  This is seen
> in valgrind as:
> 
> ==3181== Source and destination overlap in stpcpy(0x19973b48, 0x19973b48)
> ==3181==    at 0x4C2F30A: stpcpy (in
> /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==3181==    by
> 0x12567A: addgetnetgrentX (string3.h:111)
> ==3181==    by 0x12722D: addgetnetgrent (netgroupcache.c:665)
> ==3181==    by 0x11114C: nscd_run_worker (connections.c:1338)
> ==3181==    by 0x4E3C102: start_thread (pthread_create.c:309)
> ==3181==    by 0x59B81AC: clone (clone.S:111)
> ==3181==
> 
> Fix this by using memmove instead of stpcpy.  Tested x86_64 using
> various combinations of triplets (including NULL and non-NULL ones) to
> verify that this works correctly and there are no regressions.

i feel like we've wanted an equivalent of stpcpy/memccpy for memmove.  good 
time to add it ? :)

> +			    size_t hostlen = strlen (nhost ?: "") + 1;
> +			    size_t userlen = strlen (nuser ?: "") + 1;
> +			    size_t domainlen = strlen (ndomain ?: "") + 1;

we do the ?: thing a lot in this code.  time to assign a local var for it ?

>  			    char *wp = buffer + buffilled;
> -			    wp = stpcpy (wp, nhost) + 1;
> -			    wp = stpcpy (wp, nuser) + 1;
> -			    wp = stpcpy (wp, ndomain) + 1;
> +			    wp = memmove (wp, nhost ?: "", hostlen);
> +			    wp += hostlen;
> +			    wp = memmove (wp, nuser ?: "", userlen);
> +			    wp += userlen;
> +			    wp = memmove (wp, ndomain ?: "", domainlen);
> +			    wp += domainlen;

looks OK
-mike
Carlos O'Donell March 27, 2014, 8:02 a.m. UTC | #2 
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/27/2014 12:04 AM, Siddhesh Poyarekar wrote:
> Calls to stpcpy from nscd netgroups code will have overlapping source
> and destination when all three values in the returned triplet are
> non-NULL and in the expected (host,user,domain) order.  This is seen
> in valgrind as:
> 
> ==3181== Source and destination overlap in stpcpy(0x19973b48, 0x19973b48)
> ==3181==    at 0x4C2F30A: stpcpy (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==3181==    by 0x12567A: addgetnetgrentX (string3.h:111)
> ==3181==    by 0x12722D: addgetnetgrent (netgroupcache.c:665)
> ==3181==    by 0x11114C: nscd_run_worker (connections.c:1338)
> ==3181==    by 0x4E3C102: start_thread (pthread_create.c:309)
> ==3181==    by 0x59B81AC: clone (clone.S:111)
> ==3181==
> 
> Fix this by using memmove instead of stpcpy.  Tested x86_64 using
> various combinations of triplets (including NULL and non-NULL ones) to
> verify that this works correctly and there are no regressions.
> 
> Siddhesh
> 
> 	[BZ #16760]
> 	* nscd/netgroupcache.c (addgetnetgrentX): Use memmove instead
> 	of stpcpy.

OK with whitespace change removed.

Mike makes some good comments, but those cleanups can come *after*
fixing the problem.

> ---
>  nscd/netgroupcache.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
> index 5d15aa4..9999a1e 100644
> --- a/nscd/netgroupcache.c
> +++ b/nscd/netgroupcache.c
> @@ -216,6 +216,10 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>  			    const char *nuser = data.val.triple.user;
>  			    const char *ndomain = data.val.triple.domain;
>  
> +			    size_t hostlen = strlen (nhost ?: "") + 1;
> +			    size_t userlen = strlen (nuser ?: "") + 1;
> +			    size_t domainlen = strlen (ndomain ?: "") + 1;

OK.

> +
>  			    if (nhost == NULL || nuser == NULL || ndomain == NULL
>  				|| nhost > nuser || nuser > ndomain)
>  			      {
> @@ -233,9 +237,6 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>  				     : last + strlen (last) + 1 - buffer);
>  
>  				/* We have to make temporary copies.  */
> -				size_t hostlen = strlen (nhost ?: "") + 1;
> -				size_t userlen = strlen (nuser ?: "") + 1;
> -				size_t domainlen = strlen (ndomain ?: "") + 1;

OK.

>  				size_t needed = hostlen + userlen + domainlen;
>  
>  				if (buflen - req->key_len - bufused < needed)
> @@ -259,7 +260,6 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>  					       : NULL);
>  				    buffer = newbuf;
>  				  }
> -

No whitespace fixup please.

>  				nhost = memcpy (buffer + bufused,
>  						nhost ?: "", hostlen);
>  				nuser = memcpy ((char *) nhost + hostlen,
> @@ -269,9 +269,12 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>  			      }
>  
>  			    char *wp = buffer + buffilled;
> -			    wp = stpcpy (wp, nhost) + 1;
> -			    wp = stpcpy (wp, nuser) + 1;
> -			    wp = stpcpy (wp, ndomain) + 1;
> +			    wp = memmove (wp, nhost ?: "", hostlen);
> +			    wp += hostlen;
> +			    wp = memmove (wp, nuser ?: "", userlen);
> +			    wp += userlen;
> +			    wp = memmove (wp, ndomain ?: "", domainlen);
> +			    wp += domainlen;

OK.

>  			    buffilled = wp - buffer;
>  			    ++nentries;
>  			  }
> 

Cheers,
Carlos.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTM9smAAoJECXvCkNsKkr/y20H/0gzNRT5sihfoh2DQ+r87UUS
+iD4UwHVeTSJZoU43M6DZvXLm7b7Ute0OF1tsyAZc+4WDJJseoRvlz2+YPygFS+9
hquwwqBQBkqxvN1SYGAecX2bh0ePJxUl4nJoThMwQh7BdEtwfmDujBdNKXNPRJSK
Vpx6tpWwNAV2f5IQV1edNXcmJXx7hBmxXSvWTsCuijuGvx4wZRzwZ6UoZmEh7y3s
UHqQnM4RnsnYfS4znCL3Cqei8ADrj/Q7p4GLq0NScjOyP1qZ2+vBr0SPi88KGfg3
C4MzpATJlScraq5wD9iPfcTHfqKxGyD3NzJa+IWq+fml2H9wwO+UW0DTxnFWFfg=
=xHgP
-----END PGP SIGNATURE-----
Siddhesh Poyarekar March 27, 2014, 2:03 p.m. UTC | #3 
On Thu, Mar 27, 2014 at 03:34:11AM -0400, Mike Frysinger wrote:
> On Thu 27 Mar 2014 09:34:06 Siddhesh Poyarekar wrote:
> > Calls to stpcpy from nscd netgroups code will have overlapping source
> > and destination when all three values in the returned triplet are
> > non-NULL and in the expected (host,user,domain) order.  This is seen
> > in valgrind as:
> > 
> > ==3181== Source and destination overlap in stpcpy(0x19973b48, 0x19973b48)
> > ==3181==    at 0x4C2F30A: stpcpy (in
> > /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==3181==    by
> > 0x12567A: addgetnetgrentX (string3.h:111)
> > ==3181==    by 0x12722D: addgetnetgrent (netgroupcache.c:665)
> > ==3181==    by 0x11114C: nscd_run_worker (connections.c:1338)
> > ==3181==    by 0x4E3C102: start_thread (pthread_create.c:309)
> > ==3181==    by 0x59B81AC: clone (clone.S:111)
> > ==3181==
> > 
> > Fix this by using memmove instead of stpcpy.  Tested x86_64 using
> > various combinations of triplets (including NULL and non-NULL ones) to
> > verify that this works correctly and there are no regressions.
> 
> i feel like we've wanted an equivalent of stpcpy/memccpy for memmove.  good 
> time to add it ? :)

Seems like a useful thing to have.

> we do the ?: thing a lot in this code.  time to assign a local var for it ?

Yeah, I was also thinking of breaking the entire logic out into a
function of its own to improve readability, but I didn't because I
wanted the change to be minimal.  It would definitely be a good
cleanup in future.

Siddhesh
diff mbox

Patch

diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
index 5d15aa4..9999a1e 100644
--- a/nscd/netgroupcache.c
+++ b/nscd/netgroupcache.c
@@ -216,6 +216,10 @@  addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
 			    const char *nuser = data.val.triple.user;
 			    const char *ndomain = data.val.triple.domain;
 
+			    size_t hostlen = strlen (nhost ?: "") + 1;
+			    size_t userlen = strlen (nuser ?: "") + 1;
+			    size_t domainlen = strlen (ndomain ?: "") + 1;
+
 			    if (nhost == NULL || nuser == NULL || ndomain == NULL
 				|| nhost > nuser || nuser > ndomain)
 			      {
@@ -233,9 +237,6 @@  addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
 				     : last + strlen (last) + 1 - buffer);
 
 				/* We have to make temporary copies.  */
-				size_t hostlen = strlen (nhost ?: "") + 1;
-				size_t userlen = strlen (nuser ?: "") + 1;
-				size_t domainlen = strlen (ndomain ?: "") + 1;
 				size_t needed = hostlen + userlen + domainlen;
 
 				if (buflen - req->key_len - bufused < needed)
@@ -259,7 +260,6 @@  addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
 					       : NULL);
 				    buffer = newbuf;
 				  }
-
 				nhost = memcpy (buffer + bufused,
 						nhost ?: "", hostlen);
 				nuser = memcpy ((char *) nhost + hostlen,
@@ -269,9 +269,12 @@  addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
 			      }
 
 			    char *wp = buffer + buffilled;
-			    wp = stpcpy (wp, nhost) + 1;
-			    wp = stpcpy (wp, nuser) + 1;
-			    wp = stpcpy (wp, ndomain) + 1;
+			    wp = memmove (wp, nhost ?: "", hostlen);
+			    wp += hostlen;
+			    wp = memmove (wp, nuser ?: "", userlen);
+			    wp += userlen;
+			    wp = memmove (wp, ndomain ?: "", domainlen);
+			    wp += domainlen;
 			    buffilled = wp - buffer;
 			    ++nentries;
 			  }