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

Michael Niedermayer michael at niedermayer.cc
Wed Aug 31 12:37:08 EEST 2016


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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160831/58cf4698/attachment.sig>


More information about the ffmpeg-devel mailing list