[FFmpeg-devel] [PATCH] ac3_parser type punning fix
Måns Rullgård
mans
Sun Oct 19 18:23:52 CEST 2008
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?
> 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.
> 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.
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.
--
M?ns Rullg?rd
mans at mansr.com
More information about the ffmpeg-devel
mailing list