Re: [PATCH] Avoid overlapping addresses to stpcpy calls in nscd (BZ #16760)

["Carlos O'Donell" <carlos at redhat dot com>]

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