Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Message-ID: <49F228B9.7010900@apache.org>
Date: Fri, 24 Apr 2009 23:01:45 +0200
From: Ruediger Pluem <rpluem@apache.org>
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US;
 rv:1.8.1.21) Gecko/20090402 SeaMonkey/1.1.16
MIME-Version: 1.0
To: "C. Michael Pilato" <cmpilato@collab.net>
Cc: dev@apr.apache.org, Subversion Developers <dev@subversion.tigris.org>
Subject: Re: Buffer overflow in apr_brigade_vprintf() ?
References: <49F21CD2.5020105@collab.net>
In-Reply-To: <49F21CD2.5020105@collab.net>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 8bit


On 04/24/2009 10:10 PM, C. Michael Pilato wrote:

> 
>     /* tack on null terminator to remaining string */
>     *(vd.vbuff.curpos) = '\0';
> 
> Uh-oh.  Buffer overflow!
> 
> Our CollabNet engineer is proposing a simple fix:  defining 'buf' inside
> apr_brigade_vprintf() like so:
> 
>     char buf[APR_BUCKET_BUFF_SIZE + 1]
> 
> (Note the "+ 1" to make room for that pesky NULL byte.)
> 
> But I'm wondering if an equally correct fix would be to simply not tack the
> '\0' onto the buffer at all.  Doesn't apr_brigade_write() accept both the
> buffer and the number of bytes to write?  Does it really need a
> null-terminated string, especially considering that its input could be
> arbitrary binary data?  Other calls to it pass things like "str" and
> "strlen(str)", which would ignore the NULL byte in "str".
> 
> [1]
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&viewType=browseAll&dsMessageId=1745697
> 

Fixed in r768417 (http://svn.apache.org/viewvc?view=rev&revision=768417).

Regards

R�diger