You're absolutely right, I mixed everything on this one.
On Thu, Oct 17, 2013 at 7:16 PM, Jim Jagielski <[email protected]> wrote: > 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 <[email protected]> wrote: > > > On Thu, Oct 17, 2013 at 6:19 PM, Jim Jagielski <[email protected]> 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 <[email protected]> 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. > > > > > > > > >
