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

[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?

- Mark