Den fre 22 maj 2026 kl 12:04 skrev Branko Čibej <[email protected]>:
> 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.
>
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
