You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ji...@apache.org on 2013/10/17 17:02:04 UTC
svn commit: r1533100 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Author: jim
Date: Thu Oct 17 15:02:04 2013
New Revision: 1533100
URL: http://svn.apache.org/r1533100
Log:
ap_proxy_strncpy should correctly handle src being NULL.
Actually, apr_cpystrn() should as well...
Modified:
httpd/httpd/trunk/modules/proxy/proxy_util.c
Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1533100&r1=1533099&r2=1533100&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
+++ httpd/httpd/trunk/modules/proxy/proxy_util.c Thu Oct 17 15:02:04 2013
@@ -93,6 +93,11 @@ PROXY_DECLARE(apr_status_t) ap_proxy_str
char *thenil;
apr_size_t thelen;
+ /* special case: really apr_cpystrn should handle src==NULL*/
+ if (!src && dlen) {
+ *dst = '\0';
+ return APR_SUCCESS;
+ }
thenil = apr_cpystrn(dst, src, dlen);
thelen = thenil - dst;
/* Assume the typical case is smaller copying into bigger
Re: svn commit: r1533100 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Yann Ylavic <yl...@gmail.com>.
You're absolutely right, I mixed everything on this one.
On Thu, Oct 17, 2013 at 7:16 PM, Jim Jagielski <ji...@jagunet.com> 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 <yl...@gmail.com> wrote:
>
> > On Thu, Oct 17, 2013 at 6:19 PM, Jim Jagielski <ji...@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 <yl...@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.
> > >
> >
> >
>
>
Re: svn commit: r1533100 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
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 <yl...@gmail.com> wrote:
> On Thu, Oct 17, 2013 at 6:19 PM, Jim Jagielski <ji...@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 <yl...@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.
> >
>
>
Re: svn commit: r1533100 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Oct 17, 2013 at 6:43 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Thu, Oct 17, 2013 at 6:19 PM, Jim Jagielski <ji...@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.
>
Oups sorry, my bad,
I misread apr_cpystrn(), the
off-by-1 is
needed and ((thelen < dlen-1) || !src[dlen - 1]) is the correct test.
Yet the underflow when dlen is 0 is not very nice, maybe that could be
checked before calling apr_cpystrn() and turned to an error.
> Regards.
>
>
Re: svn commit: r1533100 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Oct 17, 2013 at 6:19 PM, Jim Jagielski <ji...@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 <yl...@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.
> >
>
>
Re: svn commit: r1533100 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
Need to look, but at 1st blush it looks like an off-by-1 error
there.
On Oct 17, 2013, at 11:33 AM, Yann Ylavic <yl...@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.
>
Re: svn commit: r1533100 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Oct 17, 2013 at 5:02 PM, <ji...@apache.org> wrote:
> Author: jim
> Date: Thu Oct 17 15:02:04 2013
> New Revision: 1533100
>
> URL: http://svn.apache.org/r1533100
> Log:
> ap_proxy_strncpy should correctly handle src being NULL.
> Actually, apr_cpystrn() should as well...
>
> Modified:
> httpd/httpd/trunk/modules/proxy/proxy_util.c
>
> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1533100&r1=1533099&r2=1533100&view=diff
>
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Thu Oct 17 15:02:04 2013
> @@ -93,6 +93,11 @@ PROXY_DECLARE(apr_status_t) ap_proxy_str
> char *thenil;
> apr_size_t thelen;
>
> + /* special case: really apr_cpystrn should handle src==NULL*/
> + if (!src && dlen) {
> + *dst = '\0';
> + return APR_SUCCESS;
> + }
> thenil = apr_cpystrn(dst, src, dlen);
> thelen = thenil - dst;
> /* Assume the typical case is smaller copying into bigger
>
>
>
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.