[FFmpeg-devel] [PATCH v2] Let clang-FORTIFY build; NFC.

[Ronald S. Bultje]

Hi George,

On Tue, Aug 30, 2016 at 8:47 PM, George Burgess <gbiv at chromium.org> wrote:

> Thanks for the feedback! I agree the casts aren't pretty. :)
>
> > Isn't it easier to change your fortify-clang and add a compiler option
> to disable this specific error for specific targets?
>
> The short answer is "in some cases, yes. Sadly, this doesn't seem to be
> one of those cases."
>
> The longer answer is that FORTIFY is a thing that's implemented partially
> in the compiler, and partially in the standard library (for example, the
> canonical FORTIFY implementation* has bits in both gcc and glibc). The
> errors this patch is trying to fix originate from the bits in the standard
> library, so it's not as simple as checking if the compiler got a flag. At
> this point, the least-effort fix would be turning FORTIFY off for
> ${project_with_errors}. If we wanted to be more granular, we could probably
> add #ifndef _DISABLE_FORTIFY_FOR_$functionName for each FORTIFY'ed
> function, but:
> 1. grep tells me there are currently 75 FORTIFY functions, so we would
> need 75 such flags;
> 2. it lessens the effectiveness of FORTIFY across the entire project; and
> 3. the idea of hand-curating a list of per-project+per-function defines,
> that can arbitrarily change from release to release, seems kind of ugly in
> itself. :/
>

I agree it's a little iffy. Can you explain what the goal of fortify is and
what the reason for the errors is? Most of the patch (cursory glance, not
looking at scope of variables or anything) seems to suggest that the patch
tries to prevent the compiler auto-casting between pointers of sized types
(almost always uint8_t *) and native types (almost always char *). It seems
the goal here is to remove the assumption that the two are of the same
size. I'm blindly assuming the signedness isn't relevant here, but please
feel free to correct me. Did I get that right?

If that's the case, I'm wondering if there's a chance the patch makes
things worse. It obviously doesn't introduce bugs, don't get me wrong. But
there's an issue. If the goal of fortify is to prepare sources from being
ready for situations where e.g. char=16bit, then I don't think this patch
fixes that situation. FFmpeg will still not work in that situation. It will
probably crash, but at the very least it will generate incorrect data in
these functions. The patch essentially silences compiler warnings that
would be generated related to this.

Is that the right thing to do?

(Or maybe I'm misunderstanding fortify, is there some documentation about
it? Or do you know what you're trying to accomplish with it? Is this the
same thing as FORTIFY_SOURCE?)

One reason for asking all these questions is that if we accept this patch,
we likely want to add a fate station (at least compilation) to guarantee
this keeps working in the future. That essentially makes it officially
supported. As such, it'd make sense to ensure/understand we're doing the
right thing and not just silencing some "valid warnings". In no way do I
want to suggest you don't know what you're doing, I'm sort-of trying to
verify that we understand it also.

Thanks!
Ronald