Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
MIME-Version: 1.0
Date: Wed, 23 Sep 2015 15:23:58 -0400
Message-ID: 
 <CA+7dWMGtFvoodXuntFeNFSAv=-+A0Y8u=-No6G49GDU+NT=uyQ@mail.gmail.com>
Subject: Issues/questions with apr_memcache_multigetp
From: Jeffrey Crowell <jcrowell@google.com>
To: dev@apr.apache.org
Content-Type: multipart/alternative; boundary=001a1141cd9a661f8d05206f0edf

--001a1141cd9a661f8d05206f0edf
Content-Type: text/plain; charset=UTF-8

Hi,

I work on mod_pagespeed, where we use a forked version of apr_memcache.c (
https://github.com/pagespeed/mod_pagespeed/blob/master/third_party/aprutil/apr_memcache2.c)
to interact with memcached.

Recently we've become aware of a bug where we end up hanging in
apr_memcache2_multigetp, pinning cpu at 100%.

We have some patches which were created in an attempt to fix some signed
bugs in the original code that I think may be causing our issue.

Namely here:
https://github.com/pagespeed/mod_pagespeed/blob/master/third_party/aprutil/apr_memcache2.c#L1453
 vs here
https://gist.github.com/crowell/59bfa1bb9f0cda30c48a#file-multiget-c-L192
The original code uses an atoi(), which has undefined behavior when called
on non integer strings, leaving len to be 0.

Second, the check here
https://github.com/pagespeed/mod_pagespeed/blob/master/third_party/aprutil/apr_memcache2.c#L1457
vs https://gist.github.com/crowell/59bfa1bb9f0cda30c48a#file-multiget-c-L199

In the original apr code, this check will never fail. len is an apr_size_t,
and will never be < 0.

The issue here now is that the check in the "server sent back a key that i
didn't ask for"
https://github.com/pagespeed/mod_pagespeed/blob/master/third_party/aprutil/apr_memcache2.c#L1507
(same in both forked and upstream), apr_pollset_remove, is never called,
and the number of queries sent is never changed.

Does it seem possible that this is causing the server to spin here, and
would apr be interested in accepting a patch to fix the signedness issues?

Thanks,
Jeff

--001a1141cd9a661f8d05206f0edf
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Hi,<div><br></div><div>I work on mod_pagespeed, where we u=
se a forked version of apr_memcache.c (<a href=3D"https://github.com/pagesp=
eed/mod_pagespeed/blob/master/third_party/aprutil/apr_memcache2.c">https://=
github.com/pagespeed/mod_pagespeed/blob/master/third_party/aprutil/apr_memc=
ache2.c</a>) to interact with memcached.</div><div><br></div><div>Recently =
we&#39;ve become aware of a bug where we end up hanging in apr_memcache2_mu=
ltigetp, pinning cpu at 100%.</div><div><br></div><div>We have some patches=
 which were created in an attempt to fix some signed bugs in the original c=
ode that I think may be causing our issue.</div><div><br></div><div>Namely =
here:=C2=A0<a href=3D"https://github.com/pagespeed/mod_pagespeed/blob/maste=
r/third_party/aprutil/apr_memcache2.c#L1453">https://github.com/pagespeed/m=
od_pagespeed/blob/master/third_party/aprutil/apr_memcache2.c#L1453</a> =C2=
=A0vs here=C2=A0<a href=3D"https://gist.github.com/crowell/59bfa1bb9f0cda30=
c48a#file-multiget-c-L192">https://gist.github.com/crowell/59bfa1bb9f0cda30=
c48a#file-multiget-c-L192</a></div><div>The original code uses an atoi(), w=
hich has undefined behavior when called on non integer strings, leaving len=
 to be 0.</div><div><br></div><div>Second, the check here=C2=A0<a href=3D"h=
ttps://github.com/pagespeed/mod_pagespeed/blob/master/third_party/aprutil/a=
pr_memcache2.c#L1457">https://github.com/pagespeed/mod_pagespeed/blob/maste=
r/third_party/aprutil/apr_memcache2.c#L1457</a> vs=C2=A0<a href=3D"https://=
gist.github.com/crowell/59bfa1bb9f0cda30c48a#file-multiget-c-L199">https://=
gist.github.com/crowell/59bfa1bb9f0cda30c48a#file-multiget-c-L199</a></div>=
<div><br></div><div>In the original apr code, this check will never fail. l=
en is an apr_size_t, and will never be &lt; 0.</div><div><br></div><div>The=
 issue here now is that the check in the &quot;server sent back a key that =
i didn&#39;t ask for&quot;=C2=A0<a href=3D"https://github.com/pagespeed/mod=
_pagespeed/blob/master/third_party/aprutil/apr_memcache2.c#L1507">https://g=
ithub.com/pagespeed/mod_pagespeed/blob/master/third_party/aprutil/apr_memca=
che2.c#L1507</a> (same in both forked and upstream), apr_pollset_remove, is=
 never called, and the number of queries sent is never changed.</div><div><=
br></div><div>Does it seem possible that this is causing the server to spin=
 here, and would apr be interested in accepting a patch to fix the signedne=
ss issues?</div><div><br></div><div>Thanks,</div><div>Jeff</div></div>

--001a1141cd9a661f8d05206f0edf--