Re: svn commit: r1936811 - apr/apr/trunk/memcache

On Thu, Aug 6, 2026 at 6:50 AM Ruediger Pluem <[email protected]> wrote:
>
>
>
> On 8/3/26 2:30 PM, [email protected] wrote:
> > Author: covener
> > Date: Mon Aug  3 12:30:34 2026
> > New Revision: 1936811
> >
> > Log:
> > apr_memcache: error checking
> >
> > Reviewed By: covener, jorton, jfclere
> >
> > Modified:
> >    apr/apr/trunk/memcache/apr_memcache.c
> >
> > Modified: apr/apr/trunk/memcache/apr_memcache.c
> > ==============================================================================
> > --- apr/apr/trunk/memcache/apr_memcache.c     Mon Aug  3 12:28:37 2026        (r1936810)
> > +++ apr/apr/trunk/memcache/apr_memcache.c     Mon Aug  3 12:30:34 2026        (r1936811)
> >
> > @@ -1169,9 +1174,14 @@ apr_memcache_version(apr_memcache_server
> >      }
> >
> >      if (strncmp(MS_VERSION, conn->buffer, MS_VERSION_LEN) == 0) {
> > -        *baton = apr_pstrmemdup(p, conn->buffer+MS_VERSION_LEN+1,
> > -                                conn->blen - MS_VERSION_LEN - 2);
> > -        rv = APR_SUCCESS;
> > +        if (conn->blen < MS_VERSION_LEN + 2) {
> > +            rv = APR_EGENERAL;
> > +        }
> > +        else {
> > +            *baton = apr_pstrmemdup(p, conn->buffer+MS_VERSION_LEN+1,
> > +                                    conn->blen - MS_VERSION_LEN - 2);
>
> I know that this is taken from the old code, but shouldn't it be -3 or -1 instead of -2 above?
>
> MS_VERSION is "VERSION"
> Hence MS_VERSION_LEN is 7
>
> If memcached correctly replies to the "version" command it returns:
>
> VERSION XYZ\r\n
>
> This means conn->blen is 13
>
> conn->buffer+MS_VERSION_LEN+1 = conn->buffer+7+1 = conn->buffer+8 points to the 'X'
> conn->blen - MS_VERSION_LEN - 2 is 13 - 7 - 2  = 4
>
> This would mean baton would be "XYZ\r". This sounds wrong. It should be either "XYZ" (I would prefer this,
> in this case we need -3 above) or "XYZ\r\n" (in this case we need -1 above).

I think you are right (-3).  Is the length check also off? by 2?

     if (strncmp(MS_VERSION, conn->buffer, MS_VERSION_LEN) == 0) {
       if (conn->blen < MS_VERSION_LEN + 2) {

The buffer needs space for VERSION + SP + CR + LF _and_ at least 1
character/digit.