httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jim Jagielski <...@jaguNET.com>
Subject Re: svn commit: r1533100 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Date Thu, 17 Oct 2013 17:16:39 GMT
Look at it this way: if thelen !< dlen then its
either the same or greater. Also, thelen is basically
the strlen of the string copied. If it is the size
of src, then we're fine. So the check SHOULD be

	if ((thelen < dlen-1) || !src[thelen]) {

using dlen we could blow past the actual bounds of src.
The above is safe since we know that src is at least thelen
chars (since that's how many we've copied)

On Oct 17, 2013, at 12:43 PM, Yann Ylavic <ylavic.dev@gmail.com> wrote:

> On Thu, Oct 17, 2013 at 6:19 PM, Jim Jagielski <jim@jagunet.com> wrote:
> Need to look, but at 1st blush it looks like an off-by-1 error
> there.
> 
> When source length >= dlen, apr_cpystrn() ensures dst[0:dlen - 1] == src[0:dlen -
1], hence off-by-1 is useless.
> 
> Regards.
> 
>  
> On Oct 17, 2013, at 11:33 AM, Yann Ylavic <ylavic.dev@gmail.com> wrote:
> 
> >
> > Maybe ap_proxy_strncpy() could aso have no "slow" path with this change :
> >
> > Index: modules/proxy/proxy_util.c
> > ===================================================================
> > --- modules/proxy/proxy_util.c    (revision 1533118)
> > +++ modules/proxy/proxy_util.c    (working copy)
> > @@ -90,7 +90,6 @@ APR_IMPLEMENT_OPTIONAL_HOOK_RUN_ALL(proxy, PROXY,
> >  PROXY_DECLARE(apr_status_t) ap_proxy_strncpy(char *dst, const char *src,
> >                                               apr_size_t dlen)
> >  {
> > -    char *thenil;
> >      apr_size_t thelen;
> >
> >      /* special case: really  apr_cpystrn should handle src==NULL*/
> > @@ -98,11 +97,8 @@ PROXY_DECLARE(apr_status_t) ap_proxy_strncpy(char
> >          *dst = '\0';
> >          return APR_SUCCESS;
> >      }
> > -    thenil = apr_cpystrn(dst, src, dlen);
> > -    thelen = thenil - dst;
> > -    /* Assume the typical case is smaller copying into bigger
> > -       so we have a fast return */
> > -    if ((thelen < dlen-1) || ((strlen(src)) == thelen)) {
> > +    thelen = apr_cpystrn(dst, src, dlen) - dst;
> > +    if (thelen < dlen || !src[dlen]) {
> >          return APR_SUCCESS;
> >      }
> >      /* XXX: APR_ENOSPACE would be better */
> > [EOS]
> >
> > Regards,
> > Yann.
> >
> 
> 


Mime
View raw message