[FFmpeg-devel] [PATCH] avutil: add av_memcpy() to avoid undefined behavior with NULL, NULL, 0

[Michael Niedermayer]

On Fri, Jul 05, 2019 at 05:01:33PM +0200, Tomas Härdin wrote:
> ons 2019-07-03 klockan 10:46 +0200 skrev Michael Niedermayer:
> > On Wed, Jul 03, 2019 at 09:41:41AM +0200, Reimar Döffinger wrote:
> > > 
> > > 
> > > On 03.07.2019, at 08:29, Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > 
> > > > On Tue, Jul 02, 2019 at 08:42:43PM -0300, James Almer wrote:
> > > > > 
> > > > > How many cases are there in the codebase where cnt can be 0, and dst or
> > > > > src NULL, without it having been checked before calling memcpy? And from
> > > > > those, which would not be from situations where the code should have
> > > > > instead aborted and returned ENOMEM, or EINVAL if either of them are
> > > > > function arguments?
> > > > 
> > > > There are around 2500 occurances of memcpy in the codebase
> > > > To awnser your question it would be needed to review all of them and in many
> > > > cases their surrounding code.
> > > > So that is unlikely to be awnsered by anyone accuratly
> > > > 
> > > > Also iam not sure i understand why you ask or why this would matter
> > > > the suggested function allows to simplify cases where the NULL can
> > > > occur, not where it cannot or should not. That is this is intended for
> > > > the cases where we already have or are adding explicit checks to
> > > > avoid the NULL case.
> > > > 
> > > > i could rename this to av_memcpy_nullsafe which would make it clearer but
> > > > also its more to write and read
> > > 
> > > I admit I thought that a worthwhile idea originally,
> > > but I have to think back to a long time ago that every function
> > > added to our "API" has a cost of people having to know about it and
> > > how to use it.
> > > And if it's currently only 2 places that would benefit I think
> > > James is right to ask if it makes sense.
> > > Of course another question might be if it might make sense to
> > > replace all memcpy uses with it.
> > > I mean, isn't it naturally expected behaviour that the pointers
> > > would be ignored if the copy amount is 0? There might be a lot of
> > > code assuming that we do not know about...
> > 
> > in addition to the 2 there are these related commits found by very dumb git log greps
> > In further addition there would be cases that deal with src == dst, something we
> > could add a check for in av_memcpy() too
> 
> All of these proposed "solutions" are equally horrible

> 
> I'm going to raise the issue of formal verification again (Frama-C and
> friends), because crap like this should be checked at compile time, not
> with flakey runtime checks.
> 
> I gave adding ACSL markup to parts of lavu a stab a while back, and it
> looked somewhat promising. Bit hacks present a bit of a problem
> however..

As we are already off topic, heres an example to test static
analysis, does this trigger undefined behavior by executing the memcpy
for some user input ?

void f(unsigned bigint a) {
    bigint i;
    for (i = 2; (((bigint)1 << a) + 1) % i; i++)
        ;
    if (a > 20 && i > ((bigint)1 << a))
        memcpy(NULL, NULL, 0);
}

i know its a lame example but just to remind that static analysis has
limitations. (your mail sounds a bit like static analysis could replace
everything ...)

Thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190706/30b3d1cf/attachment.sig>