[FFmpeg-devel] [PATCH] ac3_parser type punning fix

Michael Niedermayer michaelni
Sun Oct 19 18:55:43 CEST 2008


On Sun, Oct 19, 2008 at 05:23:52PM +0100, M?ns Rullg?rd wrote:
> Michael Niedermayer <michaelni at gmx.at> writes:
> 
> > On Sun, Oct 19, 2008 at 11:26:29AM +0100, M?ns Rullg?rd wrote:
> >> Michael Niedermayer <michaelni at gmx.at> writes:
> >> 
> >> > On Sat, Oct 18, 2008 at 10:44:16PM -0700, David DeHaven wrote:
> >> >> 
> >> >> > Nothing major, I just applied the same fix that was applied to  
> >> >> > aac_parser.c recently...
> >> >> >
> >> >> > -DrD-
> >> >> > <ac3_parser.patch>
> >> >> 
> >> >> I think there was confusion about the whole point of this patch...
> >> >> 
> >> >> On the four x86 based machine/OS combos I built and tested on (i686-pc- 
> >> >> mingw32, 2x i686-pc-linux, i686-apple-darwin), gcc with optimizations  
> >> >> at more than -O0 would produce machine code that would *only* byteswap  
> >> >> 32 of the 64 bits in the "state" variable passed to both aac_sync and  
> >> >> ac3_sync leading to complete and utter misinterpretation of the header  
> >> >> information. The result: at the *least* would be that it reported the  
> >> >> wrong information to stdout, at worst it would crash due to having the  
> >> >> wrong codec parameters. I can't honestly believe that noone else has  
> >> >> seen this problem, especially considering aac_sync has been already  
> >> >> fixed unless that was simply a "compiler warning" fix.
> >> >> 
> >> >> I can provide disassembly snippets if you need further convincing...
> >> >
> >> > I have no doubt that gcc generates wrong code i also have no doubt that
> >> > the code breaks strict aliassing rules.
> >> > The doubt i have is the relation, so to convince me first tell us if the
> >> > code works with all identical but -fno-strict-aliasing added. If not
> >> > the bug is elsewhere, and this patch is hiding it.
> >> 
> >> The C code is the same as in aac_parser, and I confirmed it was broken
> >> myself.  IIRC, it is only gcc 4.3 that optimises it "incorrectly".
> >> Scare quotes because the C code is what is incorrect, breaking strict
> >> aliasing rules.  You accepted the same patch for the aac parser
> >> without questions, so why the hesitation now?
> >
> > several reasons,
> > 1st i tried with and without -fno-strict-aliasing
> > and i get:
> > -00000480 <ac3_sync>:
> > +000004a0 <ac3_sync>:
> >  :      53                      push   %ebx
> >  :      83 ec 58                sub    $0x58,%esp
> >  :      8b 44 24 60             mov    0x60(%esp),%eax
> > @@ -397,12 +404,11 @@
> >  :      c7 44 24 48 00 00 00    movl   $0x0,0x48(%esp)
> >  :      00 
> >  :      89 04 24                mov    %eax,(%esp)
> > -:      e8 fc ff ff ff          call   4cc <ac3_sync+0x4c>
> > +:      e8 fc ff ff ff          call   4ec <ac3_sync+0x4c>
> >  :      31 d2                   xor    %edx,%edx
> >  :      85 c0                   test   %eax,%eax
> > -:      78 43                   js     519 <ac3_sync+0x99>
> > +:      78 43                   js     539 <ac3_sync+0x99>
> >  :      0f b7 44 24 36          movzwl 0x36(%esp),%eax
> > -:      8b 54 24 6c             mov    0x6c(%esp),%edx
> >  :      89 43 38                mov    %eax,0x38(%ebx)
> >  :      8b 44 24 38             mov    0x38(%esp),%eax
> >  :      89 43 3c                mov    %eax,0x3c(%ebx)
> > @@ -410,6 +416,7 @@
> >  :      c7 43 40 00 06 00 00    movl   $0x600,0x40(%ebx)
> >  :      89 43 34                mov    %eax,0x34(%ebx)
> >  :      31 c0                   xor    %eax,%eax
> > +:      8b 54 24 6c             mov    0x6c(%esp),%edx
> >  :      80 7c 24 1c 02          cmpb   $0x2,0x1c(%esp)
> >  :      0f 95 c0                setne  %al
> >  :      89 02                   mov    %eax,(%edx)
> > @@ -424,7 +431,7 @@
> >  :      5b                      pop    %ebx
> >  :      c3                      ret    
> >
> > ------------
> > now i didnt confirm that the asm was wrong, so i asked if david sees
> > -fno-strict-aliasing fixing the issue he has ...
> > (and above is with gcc 4.3.1)
> 
> This is 32-bit, right?

x86-32 yes


> 
> > The second reason is
> > #define AV_RN64(a) (*((const uint64_t*)(a)))
> > #define AV_WN64(a, b) *((uint64_t*)(a)) = (b)
> > ...
> > #  define AV_RB64(x)    bswap_64(AV_RN64(x))
> > #  define AV_WB64(p, d) AV_WN64(p, bswap_64(d))
> 
> Please elaborate.

in aac_parser a AV_WB64() (=uint64_t write) is used to write into a uint8_t
array that is then finally read as uint32_t.
I do not think this is allowed under strict aliasing rules.


> 
> > The third reason is that i have doubts this really is a strict
> > aliasing issue aliasing does not apply to intXY <-> int8 it just
> > applies to things not involving int8.
> 
> That's not quite what the standard says.  It says that anything can be
> accessed as a character type, nothing about the converse.
> 
> > Now the truth is we do mix 64 write and 32 bit read with a int8
> > pointer thus our code is wrong with strict aliasing but for gcc to
> > mess up it has to proof that the int8 pointer written is not read as
> > int8 or int64.  Iam surprissed it does this but at the same time
> > does then only ommit half of the int64 code. It really could ommit
> > all if it thought its never read.  Also gcc would have to analyze
> > some non static and not inlined functions to pull this trick off.
> 
> The uint8_t pointer only exists as an argument to init_get_bits(),
> which stores the pointer value, but does not dereference it, so this
> has no effect on later accesses.

yes


> 
> Looking at the code, I see one significant difference between
> aac_sync() and ac3_sync().  Whereas aac_sync() only calls inlined
> get_bits() functions (meaning gcc can know exactly what accesses are
> happening), ac3_sync() calls another function to do the parsing.  In
> light of this, I agree it's unlikely that gcc is able to exploit any
> aliasing tricks.  The disassembly above also suggests that something
> more sinister is afoot here.
> 
> The change to aac_sync() fixed a real aliasing violation, and I
> verified the assembler before and after the change.  This seems to be
> a bit different.

hmm, seems so, iam getting for aac:
 
 00000000 <aac_sync>:
 :	83 ec 1c             	sub    $0x1c,%esp
+:	8b 54 24 24          	mov    0x24(%esp),%edx
+:	8b 44 24 20          	mov    0x20(%esp),%eax
+:	0f ca                	bswap  %edx
+:	0f c8                	bswap  %eax
+:	89 54 24 08          	mov    %edx,0x8(%esp)
+:	89 44 24 0c          	mov    %eax,0xc(%esp)
 :	8b 44 24 09          	mov    0x9(%esp),%eax
 :	0f c8                	bswap  %eax
 :	c1 e8 f4             	shr    $0xf4,%eax
 :	3d ff 0f 00 00       	cmp    $0xfff,%eax
 :	89 5c 24 14          	mov    %ebx,0x14(%esp)
 :	89 74 24 18          	mov    %esi,0x18(%esp)
-:	74 15                	je     30 <aac_sync+0x30>
+:	74 11                	je     40 <aac_sync+0x40>
 :	31 c9                	xor    %ecx,%ecx
 :	89 c8                	mov    %ecx,%eax
 :	8b 5c 24 14          	mov    0x14(%esp),%ebx
...

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I wish the Xiph folks would stop pretending they've got something they
do not.  Somehow I fear this will remain a wish. -- M?ns Rullg?rd
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081019/6544da21/attachment.pgp>



More information about the ffmpeg-devel mailing list