[FFmpeg-devel] [PATCH] avcodec/hevc_ps: fix the problem of memcmp losing effectiveness

[Mark Thompson]

On 29/03/2024 15:58, Andreas Rheinhardt wrote:
> Mark Thompson:
>> On 29/03/2024 14:00, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> On 3/29/2024 10:10 AM, Mark Thompson wrote:
>>>>> On 28/03/2024 13:15, tong1.wu-at-intel.com at ffmpeg.org wrote:
>>>>>> From: Tong Wu <tong1.wu at intel.com>
>>>>>>
>>>>>> HEVCHdrParams* receives a pointer which points to a dynamically
>>>>>> allocated memory block. It causes the memcmp always returning 1.
>>>>>> Add a function to do the comparision. A condition is also added to
>>>>>> avoid malloc(0).
>>>>>>
>>>>>> Signed-off-by: Tong Wu <tong1.wu at intel.com>
>>>>>> ---
>>>>>>     libavcodec/hevc_ps.c | 20 ++++++++++++++++----
>>>>>>     libavcodec/hevc_ps.h |  4 +++-
>>>>>>     2 files changed, 19 insertions(+), 5 deletions(-)
>>>>>
>>>>> It doesn't seem like this method works at all, even before the recent
>>>>> change with the pointer.
>>>>>
>>>>> Structs can contain arbitrary padding, and any write to the struct
>>>>> makes the padding unspecified.  memcmp() is therefore never valid as a
>>>>> method of comparing after writing some fields, as done here.  (It
>>>>> could only be valid if the structs compared were made by memcpy() with
>>>>> no fields written directly.)
>>>>
>>>> The struct is zero allocated, so shouldn't the padding be exactly the
>>>> same for two equal VPSs after parsing?
>>>>
>>>
>>> In practice it is (and the current code already relied on this); yet as
>>> has already been said padding bytes take unspecified values at any store
>>> (to any member). In practice, if the compiler uses instructions that
>>> clobber the padding, the padding in both structs is clobbered in the
>>> same way.
>>
>> This seems like a strong assumption beyond that of the C specification
>> which needs to be documented.
>>
>> Are you expecting that there is no case where ABI-undefined top bits of
>> registers can leak into the padding fields, or that all functions called
>> here will necessarily set those top bits to the same values, or
>> something else?
>>
> 
> Yes, I am expecting that. That is also what the code already relied on
> before the addition of the allocated field and there have been no
> reports that this caused issues.

Huh.  Having just experimented a bit I find myself surprised by the lengths x86-64 gcc goes to with weird unaligned accesses to avoid this (e.g. to write to aligned uint8_t a[31] it may do aligned 16-byte write to a and unaligned 16-byte write to a+15, avoiding touching the padding for no clear reason).

Are you aware of any documentation from gcc or llvm about on what they guarantee here?

> This does not change that I consider it crazy to remove the parameter
> sets referencing a parameter set that is removed.

I agree, having now read the code more.  My comment was purely driven by observing the use of memcmp() on structs (an operation well-known to be very difficult to use in standard C), not by looking carefully at the rest of the code involved.

- Mark