Mailing-List: contact dev-help@qpid.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@qpid.apache.org
Content-Type: multipart/alternative;
 boundary="===============2316657987716002922=="
MIME-Version: 1.0
Subject: Re: Review Request 14442: Check length of error description before
 assigning
 to size limited buffer
From: "Gordon Sim" <gsim@redhat.com>
To: "Rafael Schloming" <rhs@apache.org>
Cc: "Chug Rolke" <crolke@redhat.com>, "Gordon Sim" <gsim@redhat.com>,
 "qpid" <dev@qpid.apache.org>
Date: Wed, 02 Oct 2013 15:28:11 -0000
Message-ID: <20131002152811.28588.13490@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Gordon Sim" <noreply@reviews.apache.org>
References: <20131002131156.28588.69151@reviews.apache.org>
In-Reply-To: <20131002131156.28588.69151@reviews.apache.org>
Reply-To: "Gordon Sim" <gsim@redhat.com>

--===============2316657987716002922==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


> On Oct. 2, 2013, 1:11 p.m., Chug Rolke wrote:
> > /proton/trunk/proton-c/src/transport/transport.c, line 772
> > <https://reviews.apache.org/r/14442/diff/1/?file=360425#file360425line772>
> >
> >     In the original code both condition->name and condition->description are vulnerable to buffer overruns.
> >     
> >     How about replacing strncat with a function that accepts the length of the destination buffer and applies the proposed fix to all instances?
> >     
> >     Windows builds complain of unsafe functions strcat, sprintf, strncpy, strncat, and getenv for precisely the reason exposed by this bug.
> 
> Rafael Schloming wrote:
>     I'd suggest replacing both name and description with pointers to pn_string_t. The pn_string_t type has been added since the original condition code was written, and is not vulnerable to this sort of buffer overrun thing. It will automatically expand as needed. I'm happy to do this if you want to assign the JIRA to me. I've been piecemeal updating all strings to use pn_string_t as I encounter various issues.

Sounds good! The JIRA is assigned to you.


- Gordon


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14442/#review26605
-----------------------------------------------------------


On Oct. 2, 2013, 11:58 a.m., Gordon Sim wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14442/
> -----------------------------------------------------------
> 
> (Updated Oct. 2, 2013, 11:58 a.m.)
> 
> 
> Review request for qpid and Rafael Schloming.
> 
> 
> Bugs: PROTON-432
>     https://issues.apache.org/jira/browse/PROTON-432
> 
> 
> Repository: qpid
> 
> 
> Description
> -------
> 
> If error description is very long it overruns the buffer and causes segfault on processing the corrupted condition information.
> 
> 
> Diffs
> -----
> 
>   /proton/trunk/proton-c/src/transport/transport.c 1527976 
> 
> Diff: https://reviews.apache.org/r/14442/diff/
> 
> 
> Testing
> -------
> 
> Fixes my test case.
> 
> python-test, c-object-tests and c-message-tests also pass
> proton-jni, proton-java and ruby-unit-test fail for me even on a clean build
> 
> 
> Thanks,
> 
> Gordon Sim
> 
>


--===============2316657987716002922==--