You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by yl...@apache.org on 2022/01/06 12:11:48 UTC
svn commit: r1896748 - in /apr/apr/branches/1.7.x: ./ include/apr_ring.h
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)
Re: svn commit: r1896748 - in /apr/apr/branches/1.7.x: ./ include/apr_ring.h
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
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 <yl...@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)
>
>