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


git-svn-id: https://svn.apache.org/repos/asf/apr/apr/branches/1.7.x@1896748 13f79535-47bb-0310-9956-ffa450edef68

1 files changed, 16 insertions, 12 deletions

diff --git a/include/apr_ring.h b/include/apr_ring.h
index eec735fcf..7280245e1 100644
--- a/include/apr_ring.h
+++ b/include/apr_ring.h
@@ -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)