> -----Ursprüngliche Nachricht-----
> Von: Niklas Edmundsson 
> Gesendet: Freitag, 22. Februar 2008 13:45
> An: dev@httpd.apache.org
> Betreff: Re: httpd 2.2.8 segfaults
> 
> On Fri, 22 Feb 2008, Plüm, Rüdiger, VF-Group wrote:
> 
> >> In general, that patch looks truly suspicious since it seems to me
> >> it's typecasting wildly and not even using its newly invented
> >> MAX_APR_SIZE_T in all places, because (apr_size_t)(-1) 
> really is the
> >> same thing, right?
> >
> > No, MAX_APR_SIZE_T and (apr_size_t)(-1) might be different 
> depending on the
> > platform. MAX_APR_SIZE_T is ~(apr_size_t)(0).
> 
> Won't both be 0xff...ff as long as apr_size_t is unsigned (which it 
> should be)? If not, the code makes even less sense..

I thought the same so far. But there seem to be platforms that we support
where this is not the case. Don't ask which platforms these are. Somebody?

> 
> Both casting signed -1 to unsigned and flipping the bits of 0 are 
> standard methods to get the max-value possible to store in a 
> variable...
> 
> > As I have overcome my confusion regarding apr_off_t / apr_size_t I 
> > hope to have a look into the problem and find a solution how to do 
> > all the casting stuff correctly.
> 
> My tip would be: less casts. If they're needed they're usually a sign 
> of bad design or a thinko somewhere.

Meanwhile I tried to clean this up in trunk. Can you please try the attached patch?

Keep in mind that MAX_APR_SIZE_T is not present in apr-util 1.2.x and that you
need to adjust this manually. Remote eyes welcome as well.

Index: apr_brigade.c
===================================================================
--- apr_brigade.c       (revision 630122)
+++ apr_brigade.c       (working copy)
@@ -97,6 +97,7 @@
     apr_bucket *e;
     const char *s;
     apr_size_t len;
+    apr_uint64_t point64;
     apr_status_t rv;

     if (point < 0) {
@@ -108,17 +109,25 @@
         return APR_SUCCESS;
     }

+    /*
+     * Try to reduce the following casting mess: We know that point will be
+     * larger equal 0 now and forever and thus that point (apr_off_t) and
+     * apr_size_t will fit into apr_uint64_t in any case.
+     */
+    point64 = (apr_uint64_t)point;
+
     APR_BRIGADE_CHECK_CONSISTENCY(b);

     for (e = APR_BRIGADE_FIRST(b);
          e != APR_BRIGADE_SENTINEL(b);
          e = APR_BUCKET_NEXT(e))
     {
-        /* For an unknown length bucket, while 'point' is beyond the possible
+        /* For an unknown length bucket, while 'point64' is beyond the possible
          * size contained in apr_size_t, read and continue...
          */
-        if ((e->length == (apr_size_t)(-1)) && (point > APR_SIZE_MAX)) {
-            /* point is too far out to simply split this bucket,
+        if ((e->length == (apr_size_t)(-1))
+            && (point64 > (apr_uint64_t)APR_SIZE_MAX)) {
+            /* point64 is too far out to simply split this bucket,
              * we must fix this bucket's size and keep going... */
             rv = apr_bucket_read(e, &s, &len, APR_BLOCK_READ);
             if (rv != APR_SUCCESS) {
@@ -126,14 +135,15 @@
                 return rv;
             }
         }
-        else if (((apr_size_t)point < e->length) || (e->length == (apr_size_t)(-1))) {
-            /* We already consumed buckets where point is beyond
+        else if ((point64 < (apr_uint64_t)e->length)
+                 || (e->length == (apr_size_t)(-1))) {
+            /* We already consumed buckets where point64 is beyond
              * our interest ( point > MAX_APR_SIZE_T ), above.
-             * Here point falls between 0 and MAX_APR_SIZE_T
+             * Here point falls between 0 and MAX_APR_SIZE_T
              * and is within this bucket, or this bucket's len
              * is undefined, so now we are ready to split it.
              * First try to split the bucket natively... */
-            if ((rv = apr_bucket_split(e, (apr_size_t)point))
+            if ((rv = apr_bucket_split(e, (apr_size_t)point64))
                     != APR_ENOTIMPL) {
                 *after_point = APR_BUCKET_NEXT(e);
                 return rv;
@@ -150,17 +160,17 @@
             /* this assumes that len == e->length, which is okay because e
              * might have been morphed by the apr_bucket_read() above, but
              * if it was, the length would have been adjusted appropriately */
-            if ((apr_size_t)point < e->length) {
+            if (point64 < (apr_uint64_t)e->length) {
                 rv = apr_bucket_split(e, (apr_size_t)point);
                 *after_point = APR_BUCKET_NEXT(e);
                 return rv;
             }
         }
-        if (point == e->length) {
+        if (point64 == (apr_uint64_t)e->length) {
             *after_point = APR_BUCKET_NEXT(e);
             return APR_SUCCESS;
         }
-        point -= e->length;
+        point64 -= (apr_uint64_t)e->length;
     }
     *after_point = APR_BRIGADE_SENTINEL(b);
     return APR_INCOMPLETE;


Regards

Rüdiger