Re: [PR] fix: five memcpy calls in buffer/apr_buffer in apr_buffer.c [apr]

--0000000000002378d6065263ea24
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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(), sinc=
e
> 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 u=
nless 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 =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, ...).

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 ;-)

Cheers,
Daniel

--0000000000002378d6065263ea24
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">Den s=C3=B6n 17 maj 2026 kl 19:47 skrev D=
aniel Sahlberg &lt;<a href=3D"mailto:[email protected]">daniel.l.=
[email protected]</a>&gt;:</div><div class=3D"gmail_quote gmail_quote_cont=
ainer"><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;=
border-left:1px solid rgb(204,204,204);padding-left:1ex">Den s=C3=B6n 17 ma=
j 2026 kl 19:14 skrev orbisai0security (via GitHub)<br>
&lt;<a href=3D"mailto:[email protected]" target=3D"_blank">[email protected]</a>&=
gt;:<br>
&gt;<br>
&gt;<br>
&gt; orbisai0security commented on PR #73:<br>
&gt; URL: <a href=3D"https://github.com/apache/apr/pull/73#issuecomment-447=
1676980" rel=3D"noreferrer" target=3D"_blank">https://github.com/apache/apr=
/pull/73#issuecomment-4471676980</a><br>
&gt;<br>
&gt;=C2=A0 =C2=A0 Thanks for the review. I agree that the current descripti=
on overstates the issue and incorrectly frames this as a confirmed critical=
 overflow.<br>
&gt;<br>
&gt;=C2=A0 =C2=A0 I=E2=80=99ll revise the PR to narrow it to defensive hard=
ening only. In particular, I=E2=80=99ll remove the =E2=80=9Cfive memcpy cal=
ls=E2=80=9D / =E2=80=9Ccritical severity=E2=80=9D language and keep only th=
e allocation-failure guard before memcpy(), since calling memcpy with a NUL=
L destination after alloc() failure would be undefined behaviour.<br>
&gt;<br>
&gt;=C2=A0 =C2=A0 For the APR_BUFFER_MAX checks, I understand your point th=
at they do not prove that src-&gt;d.mem is actually backed by src-&gt;size =
bytes, so they do not fix the claimed issue. I=E2=80=99m happy to drop thos=
e from this PR unless you think they are still useful as a separate invaria=
nt check.<br>
&gt;<br>
&gt;=C2=A0 =C2=A0 Would a smaller patch focused only on the alloc() NULL ch=
eck, 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 furth=
er and I see that apr_pmemdup seems to do the same, it allocates a buffer a=
nd 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><d=
iv><br></div><div>I realise that you can have an abort_fn in the pool and i=
f you use apr_palloc as the alloc function you will get a callback, but you=
 can&#39;t really avoid the memcpy(NULL, ...).</div><div><br></div><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><br>=
</div><div>Cheers,</div><div>Daniel</div><div><br></div></div></div>

--0000000000002378d6065263ea24--