Re: [PR] fix: five memcpy calls in buffer/apr_buffer in apr_buffer.c [apr]
Daniel Sahlberg <[email protected]> Fri, 22 May 2026 15:12:11 +0200
| Newsgroups | gmane.comp.apache.apr.devel |
|---|---|
| Message-ID | <CAMHy98MRN3EYwWY1te_E+z-FcCcFi7MGaRF8BQaJR58J0_ZYPQ@mail.gmail.com> |
--000000000000ade286065267c73e Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Den fre 22 maj 2026 kl 12:04 skrev Branko =C4=8Cibej <[email protected]>: > On 22. 5. 26 10:35, Daniel Sahlberg wrote: > > Den s=C3=B6n 17 maj 2026 kl 19:47 skrev Daniel Sahlberg < > [email protected]>: > >> Den s=C3=B6n 17 maj 2026 kl 19:14 skrev orbisai0security (via GitHub) >> <[email protected]>: >> > >> > >> > orbisai0security commented on PR #73: >> > URL: https://github.com/apache/apr/pull/73#issuecomment-4471676980 >> > >> > Thanks for the review. I agree that the current description >> overstates the issue and incorrectly frames this as a confirmed critical >> overflow. >> > >> > I=E2=80=99ll revise the PR to narrow it to defensive hardening only= . In >> particular, I=E2=80=99ll remove the =E2=80=9Cfive memcpy calls=E2=80=9D = / =E2=80=9Ccritical severity=E2=80=9D >> language and keep only the allocation-failure guard before memcpy(), sin= ce >> calling memcpy with a NULL destination after alloc() failure would be >> undefined behaviour. >> > >> > For the APR_BUFFER_MAX checks, I understand your point that they do >> not prove that src->d.mem is actually backed by src->size bytes, so they= do >> not fix the claimed issue. I=E2=80=99m happy to drop those from this PR = unless you >> think they are still useful as a separate invariant check. >> > >> > Would a smaller patch focused only on the alloc() NULL check, with >> tests/docs adjusted for expected behaviour, be acceptable? >> >> I will refer this question to the rest of dev@ >> >> Cheers, >> Daniel >> > > Any takers? This is way above my paygrade :-) > > In the meantime I digged a bit further and I see that apr_pmemdup seems t= o > do the same, it allocates a buffer and then immediately uses it: > ... > res =3D apr_palloc(a, n); > memcpy(res, m, n); > ... > > I realise that you can have an abort_fn in the pool and if you use > apr_palloc as the alloc function you will get a callback, but you can't > really avoid the memcpy(NULL, ...). > > > > The abort callback's only purpose is to allow the application to perform = a > clean-ish/safe-ish shutdown. > > > But the fact that the pattern in apr_pmemdup is the same as in > apr_buffer_cpy make me believe this is intentional. Or we have two bugs ;= -) > > > > We use the same pattern throughout. APR does not attempt to recover from > OOM conditions, such recovery is possible only in very limited > circumstances and those depend on the application. On modern platforms th= at > allow memory overcommit, the allocation may "fail" long after apr_palloc(= ) > returns a valid pointer and there's nothing the program can do about it. > The abort callback is really only used in the rare edge cases where the > system actually tells us that the allocation (will) fail(ed). > > All that said ... the pool functions do return NULL when the abort > callback isn't set. So I guess we should be checking for NULL in our code > and ... then what? Abort? There are platforms when NULL dereference doesn= 't > cause an immediate crash, so this is tricky. > I would argue that we should return NULL - then the caller has a sporting chance to do some graceful ("Sorry, no-can-do duplicating that 1GB string. Do you want to try with something smaller?"). But we have a pattern of very defensive programming at $dayjob so I may be off the mark here. Of course, the caller must be checking the return value from apr_buffer_cpy(), which they may not do since apr_buffer_cpy never returned null before. Thanks, Daniel --000000000000ade286065267c73e Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div dir=3D"ltr">Den fre 22 maj 2026 kl 12:04 skrev Branko= =C4=8Cibej <<a href=3D"mailto:[email protected]">[email protected]</a>>= ;:</div><div class=3D"gmail_quote gmail_quote_container"><blockquote class= =3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rg= b(204,204,204);padding-left:1ex"><u></u> =20 =20 =20 <div> <div>On 22. 5. 26 10:35, Daniel Sahlberg wrote:<br> </div> <blockquote type=3D"cite"> =20 <div dir=3D"ltr"> <div dir=3D"ltr">Den s=C3=B6n 17 maj 2026 kl 19:47 skrev Daniel Sahlberg <<a href=3D"mailto:[email protected]" targe= t=3D"_blank">[email protected]</a>>:</div> <div class=3D"gmail_quote"> <blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8= ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Den s=C3=B6n 17 maj 2026 kl 19:14 skrev orbisai0security (via GitHu= b)<br> <<a href=3D"mailto:[email protected]" target=3D"_blank">git@apa= che.org</a>>:<br> ><br> ><br> > orbisai0security commented on PR #73:<br> > URL: <a href=3D"https://github.com/apache/apr/pull/73#issu= ecomment-4471676980" rel=3D"noreferrer" target=3D"_blank">https://github.co= m/apache/apr/pull/73#issuecomment-4471676980</a><br> ><br> >=C2=A0 =C2=A0 Thanks for the review. I agree that the curre= nt description overstates the issue and incorrectly frames this as a confirmed critical overflow.<br> ><br> >=C2=A0 =C2=A0 I=E2=80=99ll revise the PR to narrow it to de= fensive hardening only. In particular, I=E2=80=99ll remove the =E2=80= =9Cfive memcpy calls=E2=80=9D / =E2=80=9Ccritical severity=E2=80=9D language a= nd keep only the allocation-failure guard before memcpy(), since calling memcpy with a NULL destination after alloc() failure would be undefined behaviour.<br> ><br> >=C2=A0 =C2=A0 For the APR_BUFFER_MAX checks, I understand y= our point that they do not prove that src->d.mem is actually backed by src->size bytes, so they do not fix the claimed issue. I=E2=80=99m happy to drop those from this PR unless you = think they are still useful as a separate invariant check.<br> ><br> >=C2=A0 =C2=A0 Would a smaller patch focused only on the all= oc() NULL check, with tests/docs adjusted for expected behaviour, be acceptable?<br> <br> I will refer this question to the rest of dev@<br> <br> Cheers,<br> Daniel<br> </blockquote> <div><br> </div> <div>Any takers? This is way above my paygrade :-)</div> <div><br> </div> <div>In the meantime I digged a bit further and I see that apr_pmemdup seems to do the same, it allocates a buffer and then immediately uses it:</div> <div>...</div> <div>=C2=A0 =C2=A0 res =3D apr_palloc(a, n);<br> =C2=A0 =C2=A0 memcpy(res, m, n);</div> <div>...</div> <div><br> </div> <div>I realise that you can have an abort_fn in the pool and if you use apr_palloc as the alloc function you will get a callback, but you can't really avoid the memcpy(NULL, ...).= </div> </div> </div> </blockquote> <br> <br> The abort callback's only purpose is to allow the application to perform a clean-ish/safe-ish shutdown.<br> <br> <br> <blockquote type=3D"cite"> <div dir=3D"ltr"> <div class=3D"gmail_quote"> <div>But the fact that the pattern in apr_pmemdup is the same as in apr_buffer_cpy make me believe this is intentional. Or we have two bugs ;-)</div> </div> </div> </blockquote> <br> <br> We use the same pattern throughout. APR does not attempt to recover from OOM conditions, such recovery is possible only in very limited circumstances and those depend on the application. On modern platforms that allow memory overcommit, the allocation may "fail&q= uot; long after apr_palloc() returns a valid pointer and there's nothing the program can do about it. The abort callback is really only used in the rare edge cases where the system actually tells us that the allocation (will) fail(ed).<br> <br> All that said ... the pool functions do return NULL when the abort callback isn't set. So I guess we should be checking for NULL in ou= r code and ... then what? Abort? There are platforms when NULL dereference doesn't cause an immediate crash, so this is tricky.<br= ></div></blockquote><div><br></div><div>I would argue that we should return= NULL - then the caller has a sporting chance to do some graceful ("So= rry, no-can-do duplicating that 1GB string. Do you want to try with somethi= ng smaller?"). But we have a pattern of very defensive programming at = $dayjob so I may be off the mark here. Of course, the caller must be checki= ng the return value from apr_buffer_cpy(), which they may not do since apr_= buffer_cpy never returned null before.</div><div><br></div><div>Thanks,</di= v><div>Daniel</div><div><br></div></div></div> --000000000000ade286065267c73e--