You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Ruediger Pluem <rp...@apache.org> on 2026/08/06 10:50:29 UTC
Re: svn commit: r1936811 - apr/apr/trunk/memcache
On 8/3/26 2:30 PM, covener@apache.org 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
Re: svn commit: r1936811 - apr/apr/trunk/memcache
Posted by Eric Covener <co...@gmail.com>.
On Thu, Aug 6, 2026 at 6:50 AM Ruediger Pluem <rp...@apache.org> wrote:
>
>
>
> On 8/3/26 2:30 PM, covener@apache.org 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.