This commit especially troubles me, w.r.t. semver rules for 1.7.0 -> 1.7.1, since it actually changes the behavior for the rest of 1.x, and compatibility with previous 1.x releases. If we know that this code executes correctly compiled under 1.7.1/1.8.0 against a 1.7.0 libapr.so, then it's fine. And contrariwise, the code isn't worse executed by a program compiled against apr-1.7.0 against apr 1.7.1+, INCLUDING libapr.so 1.8.0.
If that review has been completed, please confirm. Otherwise we need to reconsider until APR 2.0. Thanks, Bill On Thu, Jan 6, 2022 at 6:11 AM <yla...@apache.org> wrote: > > Author: ylavic > Date: Thu Jan 6 12:11:48 2022 > New Revision: 1896748 > > URL: http://svn.apache.org/viewvc?rev=1896748&view=rev > Log: > Merge r1896535, r1896747 from trunk: > > > apr_ring: Don't break strict-aliasing rules in APR_RING_SPLICE_{HEAD,TAIL}(). > > GCC-11 complains (see [1]) about apr_brigade_split_ex() seemingly issuing an > out of bounds access, the cause being that APR_RING_SPLICE_{HEAD,TAIL}() is > dereferencing an APR_RING_SENTINEL() and the cast there in not very aliasing > friendly (see [2] for a great explanation by Martin Sebor). > > The APR (and user code) should never dereference APR_RING_SENTINEL(), it's > fine > as a pointer only (i.e. for comparing pointers). So this commit modifies the > APR_RING_SPLICE_{HEAD,TAIL}() and APR_RING_{CONCAT,PREPEND}() macros to use > the > passed in APR_RING_HEAD's prev/next pointers directly instead of passing the > APR_RING_SENTINEL() to APR_RING_SPLICE_{BEFORE,AFTER}(). > > Semantically, this also clarifies that > APR_RING_{SPLICE,INSERT}_{BEFORE,AFTER}() > should be called for an APR_RING_ENTRY while APR_RING_SPLICE_{HEAD,TAIL}() and > APR_RING_{CONCAT,PREPEND}() are to be called with an APR_RING_HEAD. > > [1] > In file included from ./include/apr_mmap.h:28, > from ./include/apr_buckets.h:32, > from buckets/apr_brigade.c:22: > buckets/apr_brigade.c: In function ‘apr_brigade_split’: > ./include/apr_ring.h:177:43: error: array subscript ‘struct apr_bucket[0]’ is > partly outside array bounds of ‘unsigned char[32]’ [-Werror=array-bounds] > 177 | #define APR_RING_NEXT(ep, link) (ep)->link.next > | ~~~~~~~~~~^~~~~ > ./include/apr_ring.h:247:38: note: in expansion of macro ‘APR_RING_NEXT’ > 247 | APR_RING_NEXT((epN), link) = APR_RING_NEXT((lep), link); > \ > | ^~~~~~~~~~~~~ > ./include/apr_ring.h:287:9: note: in expansion of macro > ‘APR_RING_SPLICE_AFTER’ > 287 | APR_RING_SPLICE_AFTER(APR_RING_SENTINEL((hp), elem, link), > \ > | ^~~~~~~~~~~~~~~~~~~~~ > buckets/apr_brigade.c:118:9: note: in expansion of macro > ‘APR_RING_SPLICE_HEAD’ > 118 | APR_RING_SPLICE_HEAD(&a->list, e, f, apr_bucket, link); > | ^~~~~~~~~~~~~~~~~~~~ > buckets/apr_brigade.c:90:9: note: referencing an object of size 32 allocated > by ‘apr_palloc’ > 90 | b = apr_palloc(p, sizeof(*b)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > [2] https://bugzilla.redhat.com/show_bug.cgi?id=1957353#c2 > (Note that the original comment from Martin Sebor talks about the struct > "_direntry" and the global variable "anchor" which relate to some httpd > code using an APR_RING in a similar way than apr_bucket_brigade does. So > below I allowed myself to edit the original comment to replace "_direntry" > by "apr_bucket" and "anchor" by "list" (the apr_bucket_brigade's member used > as the head of the ring) to make the link with the above commit message) > " > The message is a bit cryptic but it says that the code accesses an object > (list) of some anonymous type as it was struct apr_bucket. That's invalid > because objects can only be accessed by lvalues of compatible types (or char). > > The APR_RING_ENTRY macro is defined like so: > > #define APR_RING_ENTRY(elem) \ > struct { \ > struct elem * volatile next; \ > struct elem * volatile prev; \ > } > > so given the definition: > > APR_RING_ENTRY(apr_bucket) list; > > list can only be accessed by lvalues of its (anonymous) type but the > APR_RING_SENTINEL() macro defined like so: > > #define APR_RING_SENTINEL(hp, elem, link) \ > (struct elem *)((char *)(&(hp)->next) - APR_OFFSETOF(struct elem, link)) > > casts the address of list's next member minus some constant to a pointer to > struct apr_bucket and that pointer is then used to access the prev pointer. > The anonymous struct and struct apr_bucket are unrelated and cannot be used > each other's members even if the corresponding members have the same type and > are at the same offset within the bounds of the object. > " > > > apr_ring: Follow up to r1896535: Use APR_RING_{FIRST,LAST} macros. > > hp->{next,prev} are APR_RING_{FIRST,LAST}(hp), so use them to make > APR_RING_SPLICE_{HEAD,TAIL}() read better. > > > Submitted by: ylavic > > Modified: > apr/apr/branches/1.7.x/ (props changed) > apr/apr/branches/1.7.x/include/apr_ring.h > > Propchange: apr/apr/branches/1.7.x/ > ------------------------------------------------------------------------------ > Merged /apr/apr/trunk:r1896535,1896747 > > Modified: apr/apr/branches/1.7.x/include/apr_ring.h > URL: > http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/include/apr_ring.h?rev=1896748&r1=1896747&r2=1896748&view=diff > ============================================================================== > --- apr/apr/branches/1.7.x/include/apr_ring.h (original) > +++ apr/apr/branches/1.7.x/include/apr_ring.h Thu Jan 6 12:11:48 2022 > @@ -283,9 +283,12 @@ > * @param elem The name of the element struct > * @param link The name of the APR_RING_ENTRY in the element struct > */ > -#define APR_RING_SPLICE_HEAD(hp, ep1, epN, elem, link) \ > - APR_RING_SPLICE_AFTER(APR_RING_SENTINEL((hp), elem, link), \ > - (ep1), (epN), link) > +#define APR_RING_SPLICE_HEAD(hp, ep1, epN, elem, link) do { \ > + APR_RING_PREV((ep1), link) = APR_RING_SENTINEL((hp), elem, link);\ > + APR_RING_NEXT((epN), link) = APR_RING_FIRST((hp)); \ > + APR_RING_PREV(APR_RING_FIRST((hp)), link) = (epN); \ > + APR_RING_FIRST((hp)) = (ep1); \ > + } while (0) > > /** > * Splice the sequence ep1..epN into the ring after the last element > @@ -296,9 +299,12 @@ > * @param elem The name of the element struct > * @param link The name of the APR_RING_ENTRY in the element struct > */ > -#define APR_RING_SPLICE_TAIL(hp, ep1, epN, elem, link) \ > - APR_RING_SPLICE_BEFORE(APR_RING_SENTINEL((hp), elem, link), \ > - (ep1), (epN), link) > +#define APR_RING_SPLICE_TAIL(hp, ep1, epN, elem, link) do { \ > + APR_RING_NEXT((epN), link) = APR_RING_SENTINEL((hp), elem, link);\ > + APR_RING_PREV((ep1), link) = APR_RING_LAST((hp)); \ > + APR_RING_NEXT(APR_RING_LAST((hp)), link) = (ep1); \ > + APR_RING_LAST((hp)) = (epN); \ > + } while (0) > > /** > * Insert the element nep into the ring before the first element > @@ -331,9 +337,8 @@ > */ > #define APR_RING_CONCAT(h1, h2, elem, link) do { \ > if (!APR_RING_EMPTY((h2), elem, link)) { \ > - APR_RING_SPLICE_BEFORE(APR_RING_SENTINEL((h1), elem, link), \ > - APR_RING_FIRST((h2)), \ > - APR_RING_LAST((h2)), link); \ > + APR_RING_SPLICE_TAIL((h1), APR_RING_FIRST((h2)), \ > + APR_RING_LAST((h2)), elem, link); \ > APR_RING_INIT((h2), elem, link); \ > } \ > } while (0) > @@ -347,9 +352,8 @@ > */ > #define APR_RING_PREPEND(h1, h2, elem, link) do { \ > if (!APR_RING_EMPTY((h2), elem, link)) { \ > - APR_RING_SPLICE_AFTER(APR_RING_SENTINEL((h1), elem, link), \ > - APR_RING_FIRST((h2)), \ > - APR_RING_LAST((h2)), link); \ > + APR_RING_SPLICE_HEAD((h1), APR_RING_FIRST((h2)), \ > + APR_RING_LAST((h2)), elem, link); \ > APR_RING_INIT((h2), elem, link); \ > } \ > } while (0) > >
