Re: [PR] fix: five memcpy calls in buffer/apr_buffer in apr_buffer.c [apr]
Branko Čibej <[email protected]> Fri, 22 May 2026 12:04:18 +0200
| Newsgroups | gmane.comp.apache.apr.devel |
|---|---|
| Organization | The Apache Software Foundation |
| Message-ID | <[email protected]> |
This is a multi-part message in MIME format. --------------SZEJpkc5gprBgzhJgjzsFNpd Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 22. 5. 26 10:35, Daniel Sahlberg wrote: > Den sön 17 maj 2026 kl 19:47 skrev Daniel Sahlberg > <[email protected]>: > > Den sön 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’ll revise the PR to narrow it to defensive hardening only. > In particular, I’ll remove the “five memcpy calls” / “critical > severity” language and keep only the allocation-failure guard > before memcpy(), since 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’m 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 to do the same, it allocates a buffer and then immediately uses it: > ... > res = 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 that 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. -- Brane --------------SZEJpkc5gprBgzhJgjzsFNpd Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit <!DOCTYPE html> <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body> <div class="moz-cite-prefix">On 22. 5. 26 10:35, Daniel Sahlberg wrote:<br> </div> <blockquote type="cite" cite="mid:CAMHy98Ng-pQ2iAHWbQiJLPc5hyW1WyQ-f6oAhz1pBFC_9vebPg@mail.gmail.com"> <meta http-equiv="content-type" content="text/html; charset=UTF-8"> <div dir="ltr"> <div dir="ltr">Den sön 17 maj 2026 kl 19:47 skrev Daniel Sahlberg <<a href="mailto:[email protected]" moz-do-not-send="true" class="moz-txt-link-freetext">[email protected]</a>>:</div> <div class="gmail_quote gmail_quote_container"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Den sön 17 maj 2026 kl 19:14 skrev orbisai0security (via GitHub)<br> <<a href="mailto:[email protected]" target="_blank" moz-do-not-send="true" class="moz-txt-link-freetext">[email protected]</a>>:<br> ><br> ><br> > orbisai0security commented on PR #73:<br> > URL: <a href="https://github.com/apache/apr/pull/73#issuecomment-4471676980" rel="noreferrer" target="_blank" moz-do-not-send="true" class="moz-txt-link-freetext">https://github.com/apache/apr/pull/73#issuecomment-4471676980</a><br> ><br> > Thanks for the review. I agree that the current description overstates the issue and incorrectly frames this as a confirmed critical overflow.<br> ><br> > I’ll revise the PR to narrow it to defensive hardening only. In particular, I’ll remove the “five memcpy calls” / “critical severity” language and keep only the allocation-failure guard before memcpy(), since calling memcpy with a NULL destination after alloc() failure would be undefined behaviour.<br> ><br> > 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’m happy to drop those from this PR unless you think they are still useful as a separate invariant check.<br> ><br> > Would a smaller patch focused only on the alloc() 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> res = apr_palloc(a, n);<br> 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="cite" cite="mid:CAMHy98Ng-pQ2iAHWbQiJLPc5hyW1WyQ-f6oAhz1pBFC_9vebPg@mail.gmail.com"> <div dir="ltr"> <div class="gmail_quote gmail_quote_container"> <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" 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 our code and ... then what? Abort? There are platforms when NULL dereference doesn't cause an immediate crash, so this is tricky.<br> <br> -- Brane<br> <br> </body> </html> --------------SZEJpkc5gprBgzhJgjzsFNpd--