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

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).


> +            rv = APR_SUCCESS;
> +        }
>      }
>      else {
>          rv = APR_EGENERAL;

Regards

Rüdiger