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

Baptiste Coudurier baptiste.coudurier
Sun Oct 19 21:26:31 CEST 2008


Hi guys,

Michael Niedermayer wrote:
> 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.
> 

FYI, Mans and I talked about this issue some time ago, and then I filled
a roundup issue regarding this (raw aac playback is broken)

Even at r15647 raw aac is still borken because the parser is still broken.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Smartjog USA Inc.                                http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA




More information about the ffmpeg-devel mailing list