Re: [FFmpeg-devel] [PATCH] lavfi: get rid of FF_INTERNAL_FIELDS

[Nicolas George <george@nsup.org>]

Andreas Rheinhardt (12023-02-01):
> "One special guarantee is made in order to simplify the use of unions:
> if a union contains several structures that share a common initial
> sequence (see below), and if the union object currently contains one of
> these structures, it is permitted to inspect the common initial part of
> any of them anywhere that a declaration of the completed type of the
> union is visible. Two structures share a common initial sequence if
> corresponding members have compatible types (and, for bit-fields, the
> same widths) for a sequence of one or more initial members."
> 
> But there just is no union between the FF_INTERNAL_FIELDS
> !defined(FF_INTERNAL_FIELDS) structures in the whole codebase.

It is not necessary that there is one: it is enough that there could be
one in another compilation unit, the code that handles these structures
does not know what exists in the rest of the code base. This guarantee
was made FOR unions, but it does not REQUIRE unions, it applies to
anything that is semantically equivalent to a union.

> Furthermore, said guarantee is only for inspecting, i.e. reading. For
> example, for the following two structs sharing a common initial sequence,
> 
> struct Small {
>     uint64_t foo;
>     uint32_t bar;
> };
> struct Big {
>     uint64_t foo;
>     uint32_t bar;
>     uint32_t baz;
> };
> 
> if one had a union { struct Small; struct Big; }, a write to Small.bar
> may clobber Big.baz.

That is a good point. It does not apply when the first field of Big is
Small itself, which is the case that I absolutely know is supported,
because in that case Big will embed the padding of Small. I had not
thought of this.

Fortunately, even if we are not 100% in the case that is officially
supported, there are multiple reasons that each should be enough to
guarantee that our code will work:

- Between the public part of the structure and the #ifede
  FF_INTERNAL_FIELDS, there are a lot of "not part of the public API"
  fields", changing ch_layout will not clobber incfg because incfg is
  known at this point.

- Even if there was no fields in between, reserved[] offers the same
  protection.

- And even if there was no gap, we are good because applications, and
  even filters, are not supposed to modify AVFilterLink, only the
  framework and the framework knows the whole structure.

In fact, that last remark makes me think of another solution, for the
slightly longer term: make AVFilterLink entirely opaque. The only
documented reason for application to access AVFilterLink was to get the
parameters of buffersink, but buffersink has a real API to get this
since 2016.

Anyway, if you prefer using a compilation option, I have no objection.
My only concern is that when a developer writes:

	if (link->x == ... &&
	    link->y == ... &&
	    link->status_in == ... &&
	    link->min_samples == ..)

they have to remember that no, status_in is different from all the
others.

And it is especially bad in this particular case because the distinction
is not public / private, which makes some semantic sense and might be
easier to remember, the distinction is actually
public-or-private-because-of-a-comment / private-because-of-a-ifdef.

But even if the distinction really was public / private, I consider it
unacceptable if we can do otherwise. And we can.

Regards,

-- 
  Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".