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)
Mime-Version: 1.0 (Apple Message framework v752.2)
In-Reply-To: <20071208154455.DAA851A9838@eris.apache.org>
References: <20071208154455.DAA851A9838@eris.apache.org>
Content-Type: text/plain; charset=US-ASCII; delsp=yes; format=flowed
Message-Id: <703D4357-C06F-4A73-8F30-4974B796EDAC@jaguNET.com>
Content-Transfer-Encoding: 7bit
From: Jim Jagielski <jim@jaguNET.com>
Subject: Re: svn commit: r602485 - /httpd/httpd/branches/2.2.x/STATUS
Date: Sat, 8 Dec 2007 11:11:28 -0500
To: dev@httpd.apache.org


On Dec 8, 2007, at 10:44 AM, niq@apache.org wrote:
> +    niq: You're missing my point.  That this patch fixes a bug is
> +         perfectly clear.  But it does so by allocating  
> (AP_IOBUFSIZE+1)
> +         bytes.  My point: isn't that likely to be horribly  
> inefficient
> +         if AP_IOBUFSIZE == 2^n+1?  THEREFORE this fix should be
> +         accompanied by decrementing AP_IOBUFSIZE, so that the
> +         allocation of AP_IOBUFSIZE+1 bytes becomes 2^n.
> +         Correct me if I'm wrong about that mattering ever, on
> +         any platform and architecture.
>

Nick, as I understand it, your issue is whether it is
functionally inefficient to allocate space that is not
a multiple of 2, is that right? That is a buffer of 2^n
is "better" than one which is not? Since this is a local
allocation and we're always just using AP_IOBUFSIZE
worth of the data in it, it really doesn't matter. In
fact, decreasing AP_IOBUFSIZE would be very bad because
that is a value which is used for all sorts of network
and buffering codepaths, in which case having a 2^n size
*is* crucial.

As far as the patch is concerned, the issue is that
there are conditions where vd.vbuff.curpos is actually
pointing to vrprintf_buf + AP_IOBUFSIZE, which is
1 outside of an allocation of vrprintf_buf[AP_IOBUFSIZE].
So when the

     *(vd.vbuff.curpos) = '\0';

is done, we're outside the bounds. There are 2 ways to
fix this. They current way which is simply to ensure that
vrprintf_buf + AP_IOBUFSIZE is inside the allocation OR
we can simply drop the "terminate string" line. Both address
the issue.

The reason I did the 1st is that it fixed the issue and
that I couldn't see the reason for the string-terminate line
but figured it was there for a reason anyway... I've been
able to profile all this and see that the line is really
not needed at all. So in r602491 I've changed to the
2nd fix, even though the 1st is fine.