Avoid overlapping addresses to stpcpy calls in nscd (BZ #16760)
Commit Message
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
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
-----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-----
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
@@ -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;
}