Message ID | xn7ezbx3ao.fsf@greed.delorie.com |
---|---|
State | New |
Headers | show |
On 07/13/2017 08:36 PM, DJ Delorie wrote: > Fixes https://sourceware.org/bugzilla/show_bug.cgi?id=21654 > > I have a test for this in my nss_tests patch (posted separately), > which is a very large patch set (the individual test requires the new > framework) and thus I chose to keep it as a separate patch in case > this patch was accepted during the freeze and the testuite change was > deferred. > > Tested on x86_64, where it fails the test I wrote before, and not > after, patching. > > Note: I did not try to rewrite grp_merge.c using Florian's new > alloc_buffer code, as I felt that was inappropriate for this stage of > development (post-freeze). > > [BZ #21654] > * grp/grp_merge.c (__copy_grp): Make sure pointers-to-not-char > are properly aligned. > (__merge_grp): Likewise. LGTM. I expect the test cases I just reviewed form the basis of the regression test for this fix, so this has to go in first otherwise s390x will show a test regression. > diff --git a/grp/grp-merge.c b/grp/grp-merge.c > index 77c494d..d6a53cd 100644 > --- a/grp/grp-merge.c > +++ b/grp/grp-merge.c > @@ -85,6 +85,14 @@ __copy_grp (const struct group srcgrp, const size_t buflen, > } > members[i] = NULL; > > + /* Align for pointers. We can't simply align C because we need to > + align destbuf[c]. */ > + if (((uintptr_t)destbuf + c) & (__alignof__(char **) - 1)) Avoid boolean coercion. Compare to zero. > + { > + uintptr_t mis_align = ((uintptr_t)destbuf + c) & (__alignof__(char **) - 1); > + c += __alignof__(char **) - mis_align; OK, align the value of c up such that it is properly aligned. > + } > + > /* Copy the pointers from the members array into the buffer and assign them > to the gr_mem member of destgrp. */ > destgrp->gr_mem = (char **) &destbuf[c]; > @@ -168,6 +176,14 @@ __merge_grp (struct group *savedgrp, char *savedbuf, char *savedend, > /* Add the NULL-terminator. */ > members[savedmemcount + memcount] = NULL; > > + /* Align for pointers. We can't simply align C because we need to > + align savedbuf[c]. */ > + if (((uintptr_t)savedbuf + c) & (__alignof__(char **) - 1)) > + { > + uintptr_t mis_align = ((uintptr_t)savedbuf + c) & (__alignof__(char **) - 1); > + c += __alignof__(char **) - mis_align; OK. > + } > + > /* Copy the member array back into the buffer after the member list and free > the member array. */ > savedgrp->gr_mem = (char **) &savedbuf[c]; >
diff --git a/grp/grp-merge.c b/grp/grp-merge.c index 77c494d..d6a53cd 100644 --- a/grp/grp-merge.c +++ b/grp/grp-merge.c @@ -85,6 +85,14 @@ __copy_grp (const struct group srcgrp, const size_t buflen, } members[i] = NULL; + /* Align for pointers. We can't simply align C because we need to + align destbuf[c]. */ + if (((uintptr_t)destbuf + c) & (__alignof__(char **) - 1)) + { + uintptr_t mis_align = ((uintptr_t)destbuf + c) & (__alignof__(char **) - 1); + c += __alignof__(char **) - mis_align; + } + /* Copy the pointers from the members array into the buffer and assign them to the gr_mem member of destgrp. */ destgrp->gr_mem = (char **) &destbuf[c]; @@ -168,6 +176,14 @@ __merge_grp (struct group *savedgrp, char *savedbuf, char *savedend, /* Add the NULL-terminator. */ members[savedmemcount + memcount] = NULL; + /* Align for pointers. We can't simply align C because we need to + align savedbuf[c]. */ + if (((uintptr_t)savedbuf + c) & (__alignof__(char **) - 1)) + { + uintptr_t mis_align = ((uintptr_t)savedbuf + c) & (__alignof__(char **) - 1); + c += __alignof__(char **) - mis_align; + } + /* Copy the member array back into the buffer after the member list and free the member array. */ savedgrp->gr_mem = (char **) &savedbuf[c];