Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (athena.apache.org: local policy)
Content-class: urn:content-classes:message
MIME-Version: 1.0
Content-Type: multipart/mixed;
	boundary="----_=_NextPart_001_01C8755E.FED364FF"
Subject: Re: httpd 2.2.8 segfaults
Date: Fri, 22 Feb 2008 15:27:01 +0100
Message-ID: 
 <99EA83DCDE961346AFA9B5EC33FEC08B464DBD@VF-MBX11.internal.vodafone.com>
In-Reply-To: <Pine.GSO.4.64.0802221336050.21281@kleopatra.acc.umu.se>
Thread-Topic: Re: httpd 2.2.8 segfaults
Thread-Index: Ach1UQkPKpWwr/WeSrqy8oRCCyG6sQADXtQQ
References: <Pine.GSO.4.64.0802200950090.21130@kleopatra.acc.umu.se>
 <99EA83DCDE961346AFA9B5EC33FEC08B464B7E@VF-MBX11.internal.vodafone.com>
 <Pine.GSO.4.64.0802201916540.24298@kleopatra.acc.umu.se>
 <Pine.GSO.4.64.0802212127120.16348@kleopatra.acc.umu.se>
 <47BDEE44.5060609@apache.org>
 <Pine.GSO.4.64.0802221049400.21281@kleopatra.acc.umu.se>
 <99EA83DCDE961346AFA9B5EC33FEC08B464D57@VF-MBX11.internal.vodafone.com>
 <Pine.GSO.4.64.0802221336050.21281@kleopatra.acc.umu.se>
From: =?iso-8859-1?Q?Pl=FCm=2C_R=FCdiger=2C_VF-Group?=
 <ruediger.pluem@vodafone.com>
To: <dev@httpd.apache.org>

This is a multi-part message in MIME format.

------_=_NextPart_001_01C8755E.FED364FF
Content-Type: text/plain;
	charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable

=20

> -----Urspr=FCngliche Nachricht-----
> Von: Niklas Edmundsson=20
> Gesendet: Freitag, 22. Februar 2008 13:45
> An: dev@httpd.apache.org
> Betreff: Re: httpd 2.2.8 segfaults
>=20
> On Fri, 22 Feb 2008, Pl=FCm, R=FCdiger, VF-Group wrote:
>=20
> >> 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)=20
> really is the
> >> same thing, right?
> >
> > No, MAX_APR_SIZE_T and (apr_size_t)(-1) might be different=20
> depending on the
> > platform. MAX_APR_SIZE_T is ~(apr_size_t)(0).
>=20
> Won't both be 0xff...ff as long as apr_size_t is unsigned (which it=20
> 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?

>=20
> Both casting signed -1 to unsigned and flipping the bits of 0 are=20
> standard methods to get the max-value possible to store in a=20
> variable...
>=20
> > As I have overcome my confusion regarding apr_off_t / apr_size_t I=20
> > hope to have a look into the problem and find a solution how to do=20
> > all the casting stuff correctly.
>=20
> My tip would be: less casts. If they're needed they're usually a sign=20
> 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
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- 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 =3D (apr_uint64_t)point;
+
     APR_BRIGADE_CHECK_CONSISTENCY(b);

     for (e =3D APR_BRIGADE_FIRST(b);
          e !=3D APR_BRIGADE_SENTINEL(b);
          e =3D 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 =3D=3D (apr_size_t)(-1)) && (point > =
APR_SIZE_MAX)) {
-            /* point is too far out to simply split this bucket,
+        if ((e->length =3D=3D (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 =3D apr_bucket_read(e, &s, &len, APR_BLOCK_READ);
             if (rv !=3D APR_SUCCESS) {
@@ -126,14 +135,15 @@
                 return rv;
             }
         }
-        else if (((apr_size_t)point < e->length) || (e->length =3D=3D =
(apr_size_t)(-1))) {
-            /* We already consumed buckets where point is beyond
+        else if ((point64 < (apr_uint64_t)e->length)
+                 || (e->length =3D=3D (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 =3D apr_bucket_split(e, (apr_size_t)point))
+            if ((rv =3D apr_bucket_split(e, (apr_size_t)point64))
                     !=3D APR_ENOTIMPL) {
                 *after_point =3D APR_BUCKET_NEXT(e);
                 return rv;
@@ -150,17 +160,17 @@
             /* this assumes that len =3D=3D 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 =3D apr_bucket_split(e, (apr_size_t)point);
                 *after_point =3D APR_BUCKET_NEXT(e);
                 return rv;
             }
         }
-        if (point =3D=3D e->length) {
+        if (point64 =3D=3D (apr_uint64_t)e->length) {
             *after_point =3D APR_BUCKET_NEXT(e);
             return APR_SUCCESS;
         }
-        point -=3D e->length;
+        point64 -=3D (apr_uint64_t)e->length;
     }
     *after_point =3D APR_BRIGADE_SENTINEL(b);
     return APR_INCOMPLETE;


Regards

R=FCdiger


------_=_NextPart_001_01C8755E.FED364FF
Content-Type: application/octet-stream;
	name="apr_brigade_partition_fix.diff"
Content-Transfer-Encoding: base64
Content-Description: apr_brigade_partition_fix.diff
Content-Disposition: attachment;
	filename="apr_brigade_partition_fix.diff"

SW5kZXg6IGFwcl9icmlnYWRlLmMKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gYXByX2JyaWdhZGUuYwkocmV2aXNp
b24gNjMwMTIyKQorKysgYXByX2JyaWdhZGUuYwkod29ya2luZyBjb3B5KQpAQCAtOTcsNiArOTcs
NyBAQAogICAgIGFwcl9idWNrZXQgKmU7CiAgICAgY29uc3QgY2hhciAqczsKICAgICBhcHJfc2l6
ZV90IGxlbjsKKyAgICBhcHJfdWludDY0X3QgcG9pbnQ2NDsKICAgICBhcHJfc3RhdHVzX3QgcnY7
CiAKICAgICBpZiAocG9pbnQgPCAwKSB7CkBAIC0xMDgsMTcgKzEwOSwyNSBAQAogICAgICAgICBy
ZXR1cm4gQVBSX1NVQ0NFU1M7CiAgICAgfQogCisgICAgLyoKKyAgICAgKiBUcnkgdG8gcmVkdWNl
IHRoZSBmb2xsb3dpbmcgY2FzdGluZyBtZXNzOiBXZSBrbm93IHRoYXQgcG9pbnQgd2lsbCBiZQor
ICAgICAqIGxhcmdlciBlcXVhbCAwIG5vdyBhbmQgZm9yZXZlciBhbmQgdGh1cyB0aGF0IHBvaW50
IChhcHJfb2ZmX3QpIGFuZAorICAgICAqIGFwcl9zaXplX3Qgd2lsbCBmaXQgaW50byBhcHJfdWlu
dDY0X3QgaW4gYW55IGNhc2UuCisgICAgICovCisgICAgcG9pbnQ2NCA9IChhcHJfdWludDY0X3Qp
cG9pbnQ7CisKICAgICBBUFJfQlJJR0FERV9DSEVDS19DT05TSVNURU5DWShiKTsKIAogICAgIGZv
ciAoZSA9IEFQUl9CUklHQURFX0ZJUlNUKGIpOwogICAgICAgICAgZSAhPSBBUFJfQlJJR0FERV9T
RU5USU5FTChiKTsKICAgICAgICAgIGUgPSBBUFJfQlVDS0VUX05FWFQoZSkpCiAgICAgewotICAg
ICAgICAvKiBGb3IgYW4gdW5rbm93biBsZW5ndGggYnVja2V0LCB3aGlsZSAncG9pbnQnIGlzIGJl
eW9uZCB0aGUgcG9zc2libGUKKyAgICAgICAgLyogRm9yIGFuIHVua25vd24gbGVuZ3RoIGJ1Y2tl
dCwgd2hpbGUgJ3BvaW50NjQnIGlzIGJleW9uZCB0aGUgcG9zc2libGUKICAgICAgICAgICogc2l6
ZSBjb250YWluZWQgaW4gYXByX3NpemVfdCwgcmVhZCBhbmQgY29udGludWUuLi4KICAgICAgICAg
ICovCi0gICAgICAgIGlmICgoZS0+bGVuZ3RoID09IChhcHJfc2l6ZV90KSgtMSkpICYmIChwb2lu
dCA+IEFQUl9TSVpFX01BWCkpIHsKLSAgICAgICAgICAgIC8qIHBvaW50IGlzIHRvbyBmYXIgb3V0
IHRvIHNpbXBseSBzcGxpdCB0aGlzIGJ1Y2tldCwKKyAgICAgICAgaWYgKChlLT5sZW5ndGggPT0g
KGFwcl9zaXplX3QpKC0xKSkKKyAgICAgICAgICAgICYmIChwb2ludDY0ID4gKGFwcl91aW50NjRf
dClBUFJfU0laRV9NQVgpKSB7CisgICAgICAgICAgICAvKiBwb2ludDY0IGlzIHRvbyBmYXIgb3V0
IHRvIHNpbXBseSBzcGxpdCB0aGlzIGJ1Y2tldCwKICAgICAgICAgICAgICAqIHdlIG11c3QgZml4
IHRoaXMgYnVja2V0J3Mgc2l6ZSBhbmQga2VlcCBnb2luZy4uLiAqLwogICAgICAgICAgICAgcnYg
PSBhcHJfYnVja2V0X3JlYWQoZSwgJnMsICZsZW4sIEFQUl9CTE9DS19SRUFEKTsKICAgICAgICAg
ICAgIGlmIChydiAhPSBBUFJfU1VDQ0VTUykgewpAQCAtMTI2LDE0ICsxMzUsMTUgQEAKICAgICAg
ICAgICAgICAgICByZXR1cm4gcnY7CiAgICAgICAgICAgICB9CiAgICAgICAgIH0KLSAgICAgICAg
ZWxzZSBpZiAoKChhcHJfc2l6ZV90KXBvaW50IDwgZS0+bGVuZ3RoKSB8fCAoZS0+bGVuZ3RoID09
IChhcHJfc2l6ZV90KSgtMSkpKSB7Ci0gICAgICAgICAgICAvKiBXZSBhbHJlYWR5IGNvbnN1bWVk
IGJ1Y2tldHMgd2hlcmUgcG9pbnQgaXMgYmV5b25kIAorICAgICAgICBlbHNlIGlmICgocG9pbnQ2
NCA8IChhcHJfdWludDY0X3QpZS0+bGVuZ3RoKQorICAgICAgICAgICAgICAgICB8fCAoZS0+bGVu
Z3RoID09IChhcHJfc2l6ZV90KSgtMSkpKSB7CisgICAgICAgICAgICAvKiBXZSBhbHJlYWR5IGNv
bnN1bWVkIGJ1Y2tldHMgd2hlcmUgcG9pbnQ2NCBpcyBiZXlvbmQKICAgICAgICAgICAgICAqIG91
ciBpbnRlcmVzdCAoIHBvaW50ID4gTUFYX0FQUl9TSVpFX1QgKSwgYWJvdmUuCi0gICAgICAgICAg
ICAgKiBIZXJlIHBvaW50IGZhbGxzIGJldHdlZW4gMCBhbmQgTUFYX0FQUl9TSVpFX1QgCisgICAg
ICAgICAgICAgKiBIZXJlIHBvaW50IGZhbGxzIGJldHdlZW4gMCBhbmQgTUFYX0FQUl9TSVpFX1QK
ICAgICAgICAgICAgICAqIGFuZCBpcyB3aXRoaW4gdGhpcyBidWNrZXQsIG9yIHRoaXMgYnVja2V0
J3MgbGVuCiAgICAgICAgICAgICAgKiBpcyB1bmRlZmluZWQsIHNvIG5vdyB3ZSBhcmUgcmVhZHkg
dG8gc3BsaXQgaXQuCiAgICAgICAgICAgICAgKiBGaXJzdCB0cnkgdG8gc3BsaXQgdGhlIGJ1Y2tl
dCBuYXRpdmVseS4uLiAqLwotICAgICAgICAgICAgaWYgKChydiA9IGFwcl9idWNrZXRfc3BsaXQo
ZSwgKGFwcl9zaXplX3QpcG9pbnQpKSAKKyAgICAgICAgICAgIGlmICgocnYgPSBhcHJfYnVja2V0
X3NwbGl0KGUsIChhcHJfc2l6ZV90KXBvaW50NjQpKSAKICAgICAgICAgICAgICAgICAgICAgIT0g
QVBSX0VOT1RJTVBMKSB7CiAgICAgICAgICAgICAgICAgKmFmdGVyX3BvaW50ID0gQVBSX0JVQ0tF
VF9ORVhUKGUpOwogICAgICAgICAgICAgICAgIHJldHVybiBydjsKQEAgLTE1MCwxNyArMTYwLDE3
IEBACiAgICAgICAgICAgICAvKiB0aGlzIGFzc3VtZXMgdGhhdCBsZW4gPT0gZS0+bGVuZ3RoLCB3
aGljaCBpcyBva2F5IGJlY2F1c2UgZQogICAgICAgICAgICAgICogbWlnaHQgaGF2ZSBiZWVuIG1v
cnBoZWQgYnkgdGhlIGFwcl9idWNrZXRfcmVhZCgpIGFib3ZlLCBidXQKICAgICAgICAgICAgICAq
IGlmIGl0IHdhcywgdGhlIGxlbmd0aCB3b3VsZCBoYXZlIGJlZW4gYWRqdXN0ZWQgYXBwcm9wcmlh
dGVseSAqLwotICAgICAgICAgICAgaWYgKChhcHJfc2l6ZV90KXBvaW50IDwgZS0+bGVuZ3RoKSB7
CisgICAgICAgICAgICBpZiAocG9pbnQ2NCA8IChhcHJfdWludDY0X3QpZS0+bGVuZ3RoKSB7CiAg
ICAgICAgICAgICAgICAgcnYgPSBhcHJfYnVja2V0X3NwbGl0KGUsIChhcHJfc2l6ZV90KXBvaW50
KTsKICAgICAgICAgICAgICAgICAqYWZ0ZXJfcG9pbnQgPSBBUFJfQlVDS0VUX05FWFQoZSk7CiAg
ICAgICAgICAgICAgICAgcmV0dXJuIHJ2OwogICAgICAgICAgICAgfQogICAgICAgICB9Ci0gICAg
ICAgIGlmIChwb2ludCA9PSBlLT5sZW5ndGgpIHsKKyAgICAgICAgaWYgKHBvaW50NjQgPT0gKGFw
cl91aW50NjRfdCllLT5sZW5ndGgpIHsKICAgICAgICAgICAgICphZnRlcl9wb2ludCA9IEFQUl9C
VUNLRVRfTkVYVChlKTsKICAgICAgICAgICAgIHJldHVybiBBUFJfU1VDQ0VTUzsKICAgICAgICAg
fQotICAgICAgICBwb2ludCAtPSBlLT5sZW5ndGg7CisgICAgICAgIHBvaW50NjQgLT0gKGFwcl91
aW50NjRfdCllLT5sZW5ndGg7CiAgICAgfQogICAgICphZnRlcl9wb2ludCA9IEFQUl9CUklHQURF
X1NFTlRJTkVMKGIpOyAKICAgICByZXR1cm4gQVBSX0lOQ09NUExFVEU7Cg==

------_=_NextPart_001_01C8755E.FED364FF--