[FFmpeg-devel] [PATCH 1/2] AAC: fix strict aliasing violation in parser

[Eli Friedman]

On Sun, Dec 14, 2008 at 2:56 PM, M?ns Rullg?rd <mans at mansr.com> wrote:
> "Eli Friedman" <eli.friedman at gmail.com> writes:
>
>> On Sun, Dec 14, 2008 at 12:53 PM, Mans Rullgard <mans at mansr.com> wrote:
>>> -    uint8_t tmp[8];
>>> +    union {
>>> +        uint64_t u64;
>>> +        uint8_t  u8[8];
>>> +    } tmp;
>>>
>>> -    AV_WB64(tmp, state);
>>> -    init_get_bits(&bits, tmp+8-AAC_HEADER_SIZE, AAC_HEADER_SIZE * 8);
>>> +    tmp.u64 = be2me_64(state);
>>> +    init_get_bits(&bits, tmp.u8+8-AAC_HEADER_SIZE, AAC_HEADER_SIZE * 8);
>>
>> As far as I can tell, this patch does nothing except trick gcc...
>> uint8_t isn't technically guaranteed to be a character type, but in
>> practice, it always is, so uint8_t loads should always be safe.
>
> That is not the issue.  While it is safe to access a non-char type
> through a pointer to char, the converse is not true.  However,
> Corrigendum 3 (2007) to the C99 standard adds this text:
>
>   If the member used to access the contents of a union object is not
>   the same as the member last used to store a value in the object,
>   the appropriate part of the object representation of the value is
>   reinterpreted as an object representation in the new type as
>   described in 6.2.6 (a process sometimes called "type
>   punning"). This might be a trap representation.
>
> I am uncertain whether this also covers accesses through a pointer to
> a union member cast to a different type of pointer.

Oh, wait, sorry, I think I misunderstood the issue you were trying to
fix... this is trying to solve the issue with *writing* to tmp, not
reading it, right?  In that case, the patch looks correct, but it
would probably be better to fix the AV_WB64 macro so it's safe in
cases like this.

-Eli