[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