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

[George Burgess]

> I'm blindly assuming the signedness isn't relevant here

That's actually what matters here, sadly. The only way we've been able to
get FORTIFY on clang to work nearly as well as FORTIFY on gcc is with
overloading (via clang's __attribute__((overloadable)) ). This attribute is
specced to use C++'s overload resolution rules, and C++ doesn't allow for
implicitly converting between unsigned char* and char*. (AFAICT, neither
does C, but most C compilers are happy to accept it anyway)

The more I think about this, though, the more it sounds like a reasonable
idea to extend that attribute to allow these kinds of conversions if we're
in C. I'll see if clang will accept this + if we can do that without
breaking existing users of overloadable. If so, this patch won't be
necessary. If not, I'll come back with a hopefully-better approach for this.

> Can you explain what the goal of fortify is and what the reason for the
errors is?

Yeah, I could've been more clear about that -- apologies.

> Is this the same thing as FORTIFY_SOURCE?

Precisely. :) The goal is to make FORTIFY_SOURCE work better on clang. As
far as docs, there seem to be a few blog posts about what FORTIFY is meant
to do. I'm unsure if there's an official piece of documentation describing
how it works for gcc+glibc, though. I have a doc (that's still *very* much
a draft) describing it here if you're interested: https://docs.googl
e.com/document/d/1DFfZDICTbL7RqS74wJVIJ-YnjQOj1SaoqfhbgddFYS
M/edit?usp=sharing .

> 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

Yeah, the testing story for these changes is bad at the moment. :/ After a
bit more refinement (and a lot of testing), the goal is to upstream these
FORTIFY changes into glibc. If that works out well, testing will hopefully
be simple ("does it compile with clang version >N and glibc >M? If yes,
yay!"), but like you said earlier, fixing this in the compiler -- if
possible -- is probably our best bet.

> In no way do I want to suggest you don't know what you're doing

FWIW, I didn't get the "you don't know what you're doing" vibe at all from
either of your messages. :)

Thank you both for your time!
George

On Wed, Aug 31, 2016 at 2:37 AM, Michael Niedermayer <michael at niedermayer.cc
> wrote:

> On Wed, Aug 31, 2016 at 04:24:36AM -0400, Ronald S. Bultje wrote:
> > 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
>
> I wonder if a 16bit char system could even have uint8_t
> sizeof(char) is fixed at 1 by the standard, if that represents 16bit
> uint8_t should be stored as 16bit too
> but quite possibly iam missing something
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Many that live deserve death. And some that die deserve life. Can you give
> it to them? Then do not be too eager to deal out death in judgement. For
> even the very wise cannot see all ends. -- Gandalf
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>