Created attachment 30459 [details] Rewords the comments for bucket insert macros. In the apr_buckets.h header the following macros have a mismatch between the comments and the implementation: APR_BRIGADE_INSERT_HEAD, APR_BRIGADE_INSERT_TAIL, APR_BUCKET_INSERT_BEFORE and APR_BUCKET_INSERT_AFTER The comments for each of these macros mention inserting a list of buckets into a brigade. But the implementation in each case invokes an APR_RING_INSERT_* macro which inserts a single element (not a list of elements). If it were truly the intent to insert a list of buckets, some form of APR_RING_SPLICE_* macros would be used. I have attached a patch for rewording the comments from inserting a list to inserting a single bucket. If the comments were taken at face value and a list insert was attempted, the specified bucket would be correctly added to the brigade, but the remaining list would be mangled.
apr_bucket itself can be a list. (See the first field of the structure.) Maybe there are some valid improvements in the patch. If you think so, please reopen the bug with a new patch.
Hello Jeff, Yes, I am aware that apr_bucket can be a list. But it is my understanding of the code that trying to insert a bucket list of more than one bucket into a brigade will not work correctly with the listed macros. I could be wrong or just did not explain my point very well. Let's use APR_BUCKET_INSERT_BEFORE as an example: struct apr_bucket { /** Links to the rest of the brigade */ APR_RING_ENTRY(apr_bucket) link; * * * * }; #define APR_RING_ENTRY(elem) \ struct { \ struct elem * volatile next; \ struct elem * volatile prev; \ } /** * Insert a list of buckets before a specified bucket * @param a The bucket to insert before * @param b The buckets to insert */ #define APR_BUCKET_INSERT_BEFORE(a, b) do { \ apr_bucket *ap__a = (a), *ap__b = (b); \ APR_RING_INSERT_BEFORE(ap__a, ap__b, link); \ APR_BUCKET_CHECK_CONSISTENCY(ap__a); \ } while (0) /** * Insert the element nep into the ring before element lep * (..lep.. becomes ..nep..lep..) * @warning This doesn't work for inserting before the first element or on * empty rings... see APR_RING_INSERT_HEAD for one that does * @param lep Element in the ring to insert before * @param nep Element to insert * @param link The name of the APR_RING_ENTRY in the element struct */ #define APR_RING_INSERT_BEFORE(lep, nep, link) \ APR_RING_SPLICE_BEFORE((lep), (nep), (nep), link) /** * Splice the sequence ep1..epN into the ring before element lep * (..lep.. becomes ..ep1..epN..lep..) * @warning This doesn't work for splicing before the first element or on * empty rings... see APR_RING_SPLICE_HEAD for one that does * @param lep Element in the ring to splice before * @param ep1 First element in the sequence to splice in * @param epN Last element in the sequence to splice in * @param link The name of the APR_RING_ENTRY in the element struct */ #define APR_RING_SPLICE_BEFORE(lep, ep1, epN, link) do { \ APR_RING_NEXT((epN), link) = (lep); \ APR_RING_PREV((ep1), link) = APR_RING_PREV((lep), link); \ APR_RING_NEXT(APR_RING_PREV((lep), link), link) = (ep1); \ APR_RING_PREV((lep), link) = (epN); \ } while (0) Let's say we have a brigade that we want to insert into which contains buckets a1, a2, a3, etc. And we have a bucket b1 that points to b2, b3, ..., bn. bn points back to b1. Now let's try to insert the bucket list starting with b1 into brigade 'a' before bucket a2. APR_BUCKET_INSERT_BEFORE(a2, b1) This becomes APR_RING_SPLICE_BEFORE(a2, b1, b1, link). Before this macro is executed, (b1)->link.next points to b2, and (b1)->link.prev points to bn. After the macro is executed (b1)->link.next will point to a2, and (b1)->link.prev will point to a1. So only b1 is added to the brigade 'a' not the entire 'b' list. b1 is no longer connected to b2, b3, ..., bn. but (b2)->link.prev and (bn)->link.next still point to b1. From b1's prespective b2, b3, ..., bn no longer exist. Yet b2, b3, ..., bn still think that they belong to b1. Ring 'b' has been mangled into a weird sort of figure eight. This is why I think the comments are wrong to mention inserting a list of buckets. If we truly want to insert the list beginning with b1 before a2, we would need to do APR_RING_SPLICE_BEFORE(a2, b1, bn, link). If this reasoning makes sense, please, consider reopening this bug. Thanks, Mike
you're right/sorry! (I'm not touching this anymore ;) ) Maybe the comment should say "Insert a single bucket" to be even more clear for dummies like me. Someone else should chime in.
Created attachment 30485 [details] Changes the wording to a single bucket. Adopting Jeff's suggestion to change the wording to a single bucket. Perhaps no one has attempted to use these macros for inserting more than a single bucket before. Or else this mismatch would have been caught before now. Thanks for reconsidering this bug.
Fixed in trunk in r1531009... Fixed in APR-Util 1.5.x branch in r1531010... Thanks!