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

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

[tong1.wu-at-intel.com]

From: Tong Wu <tong1.wu@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@intel.com>
---
 libavcodec/hevc_ps.c | 20 ++++++++++++++++----
 libavcodec/hevc_ps.h |  4 +++-
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
index cbef3ef4cd..8b3a27a17c 100644
--- a/libavcodec/hevc_ps.c
+++ b/libavcodec/hevc_ps.c
@@ -449,6 +449,16 @@ static void uninit_vps(FFRefStructOpaque opaque, void *obj)
     av_freep(&vps->hdr);
 }
 
+static int compare_vps(const HEVCVPS *vps1, const HEVCVPS *vps2)
+{
+    if ((!vps1->hdr && !vps2->hdr) ||
+        (vps1->hdr && vps2->hdr && !memcmp(vps1->hdr, vps2->hdr, sizeof(*vps1->hdr)))) {
+        return !memcmp(vps1, vps2, offsetof(HEVCVPS, hdr));
+    }
+
+    return 0;
+}
+
 int ff_hevc_decode_nal_vps(GetBitContext *gb, AVCodecContext *avctx,
                            HEVCParamSets *ps)
 {
@@ -545,9 +555,11 @@ int ff_hevc_decode_nal_vps(GetBitContext *gb, AVCodecContext *avctx,
             goto err;
         }
 
-        vps->hdr = av_calloc(vps->vps_num_hrd_parameters, sizeof(*vps->hdr));
-        if (!vps->hdr)
-            goto err;
+        if (vps->vps_num_hrd_parameters) {
+            vps->hdr = av_calloc(vps->vps_num_hrd_parameters, sizeof(*vps->hdr));
+            if (!vps->hdr)
+                goto err;
+        }
 
         for (i = 0; i < vps->vps_num_hrd_parameters; i++) {
             int common_inf_present = 1;
@@ -569,7 +581,7 @@ int ff_hevc_decode_nal_vps(GetBitContext *gb, AVCodecContext *avctx,
     }
 
     if (ps->vps_list[vps_id] &&
-        !memcmp(ps->vps_list[vps_id], vps, sizeof(*vps))) {
+        compare_vps(ps->vps_list[vps_id], vps)) {
         ff_refstruct_unref(&vps);
     } else {
         remove_vps(ps, vps_id);
diff --git a/libavcodec/hevc_ps.h b/libavcodec/hevc_ps.h
index cc75aeb8d3..0d8eaf2b3e 100644
--- a/libavcodec/hevc_ps.h
+++ b/libavcodec/hevc_ps.h
@@ -153,7 +153,6 @@ typedef struct PTL {
 
 typedef struct HEVCVPS {
     unsigned int vps_id;
-    HEVCHdrParams *hdr;
 
     uint8_t vps_temporal_id_nesting_flag;
     int vps_max_layers;
@@ -175,6 +174,9 @@ typedef struct HEVCVPS {
 
     uint8_t data[4096];
     int data_size;
+    /* Put this at the end of the structure to make it easier to calculate the
+     * size before this pointer, which is used for memcmp */
+    HEVCHdrParams *hdr;
 } HEVCVPS;
 
 typedef struct ScalingList {
-- 
2.41.0.windows.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[James Almer]

On 3/28/2024 10:15 AM, tong1.wu-at-intel.com@ffmpeg.org wrote:
> From: Tong Wu <tong1.wu@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@intel.com>
> ---
>   libavcodec/hevc_ps.c | 20 ++++++++++++++++----
>   libavcodec/hevc_ps.h |  4 +++-
>   2 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> index cbef3ef4cd..8b3a27a17c 100644
> --- a/libavcodec/hevc_ps.c
> +++ b/libavcodec/hevc_ps.c
> @@ -449,6 +449,16 @@ static void uninit_vps(FFRefStructOpaque opaque, void *obj)
>       av_freep(&vps->hdr);
>   }
>   
> +static int compare_vps(const HEVCVPS *vps1, const HEVCVPS *vps2)
> +{
> +    if ((!vps1->hdr && !vps2->hdr) ||
> +        (vps1->hdr && vps2->hdr && !memcmp(vps1->hdr, vps2->hdr, sizeof(*vps1->hdr)))) {

I think this should be vps1->vps_num_hrd_parameters * 
sizeof(*vps1->hdr), and done after the memcmp below to ensure 
vps_num_hrd_parameters is the same value in both structs.

> +        return !memcmp(vps1, vps2, offsetof(HEVCVPS, hdr));
> +    }
> +
> +    return 0;
> +}
> +
>   int ff_hevc_decode_nal_vps(GetBitContext *gb, AVCodecContext *avctx,
>                              HEVCParamSets *ps)
>   {
> @@ -545,9 +555,11 @@ int ff_hevc_decode_nal_vps(GetBitContext *gb, AVCodecContext *avctx,
>               goto err;
>           }
>   
> -        vps->hdr = av_calloc(vps->vps_num_hrd_parameters, sizeof(*vps->hdr));
> -        if (!vps->hdr)
> -            goto err;
> +        if (vps->vps_num_hrd_parameters) {
> +            vps->hdr = av_calloc(vps->vps_num_hrd_parameters, sizeof(*vps->hdr));
> +            if (!vps->hdr)
> +                goto err;
> +        }
>   
>           for (i = 0; i < vps->vps_num_hrd_parameters; i++) {
>               int common_inf_present = 1;
> @@ -569,7 +581,7 @@ int ff_hevc_decode_nal_vps(GetBitContext *gb, AVCodecContext *avctx,
>       }
>   
>       if (ps->vps_list[vps_id] &&
> -        !memcmp(ps->vps_list[vps_id], vps, sizeof(*vps))) {
> +        compare_vps(ps->vps_list[vps_id], vps)) {
>           ff_refstruct_unref(&vps);
>       } else {
>           remove_vps(ps, vps_id);
> diff --git a/libavcodec/hevc_ps.h b/libavcodec/hevc_ps.h
> index cc75aeb8d3..0d8eaf2b3e 100644
> --- a/libavcodec/hevc_ps.h
> +++ b/libavcodec/hevc_ps.h
> @@ -153,7 +153,6 @@ typedef struct PTL {
>   
>   typedef struct HEVCVPS {
>       unsigned int vps_id;
> -    HEVCHdrParams *hdr;
>   
>       uint8_t vps_temporal_id_nesting_flag;
>       int vps_max_layers;
> @@ -175,6 +174,9 @@ typedef struct HEVCVPS {
>   
>       uint8_t data[4096];
>       int data_size;
> +    /* Put this at the end of the structure to make it easier to calculate the
> +     * size before this pointer, which is used for memcmp */
> +    HEVCHdrParams *hdr;
>   } HEVCVPS;
>   
>   typedef struct ScalingList {
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Wu, Tong1]

>From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of James
>Almer
>Sent: Friday, March 29, 2024 8:46 PM
>To: ffmpeg-devel@ffmpeg.org
>Subject: Re: [FFmpeg-devel] [PATCH] avcodec/hevc_ps: fix the problem of
>memcmp losing effectiveness
>
>On 3/28/2024 10:15 AM, tong1.wu-at-intel.com@ffmpeg.org wrote:
>> From: Tong Wu <tong1.wu@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@intel.com>
>> ---
>>   libavcodec/hevc_ps.c | 20 ++++++++++++++++----
>>   libavcodec/hevc_ps.h |  4 +++-
>>   2 files changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
>> index cbef3ef4cd..8b3a27a17c 100644
>> --- a/libavcodec/hevc_ps.c
>> +++ b/libavcodec/hevc_ps.c
>> @@ -449,6 +449,16 @@ static void uninit_vps(FFRefStructOpaque opaque,
>void *obj)
>>       av_freep(&vps->hdr);
>>   }
>>
>> +static int compare_vps(const HEVCVPS *vps1, const HEVCVPS *vps2)
>> +{
>> +    if ((!vps1->hdr && !vps2->hdr) ||
>> +        (vps1->hdr && vps2->hdr && !memcmp(vps1->hdr, vps2->hdr,
>sizeof(*vps1->hdr)))) {
>
>I think this should be vps1->vps_num_hrd_parameters *
>sizeof(*vps1->hdr), and done after the memcmp below to ensure
>vps_num_hrd_parameters is the same value in both structs.

Updated as suggested. Thanks.

Tong

>
>> +        return !memcmp(vps1, vps2, offsetof(HEVCVPS, hdr));
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   int ff_hevc_decode_nal_vps(GetBitContext *gb, AVCodecContext *avctx,
>>                              HEVCParamSets *ps)
>>   {
>> @@ -545,9 +555,11 @@ int ff_hevc_decode_nal_vps(GetBitContext *gb,
>AVCodecContext *avctx,
>>               goto err;
>>           }
>>
>> -        vps->hdr = av_calloc(vps->vps_num_hrd_parameters, sizeof(*vps-
>>hdr));
>> -        if (!vps->hdr)
>> -            goto err;
>> +        if (vps->vps_num_hrd_parameters) {
>> +            vps->hdr = av_calloc(vps->vps_num_hrd_parameters, sizeof(*vps-
>>hdr));
>> +            if (!vps->hdr)
>> +                goto err;
>> +        }
>>
>>           for (i = 0; i < vps->vps_num_hrd_parameters; i++) {
>>               int common_inf_present = 1;
>> @@ -569,7 +581,7 @@ int ff_hevc_decode_nal_vps(GetBitContext *gb,
>AVCodecContext *avctx,
>>       }
>>
>>       if (ps->vps_list[vps_id] &&
>> -        !memcmp(ps->vps_list[vps_id], vps, sizeof(*vps))) {
>> +        compare_vps(ps->vps_list[vps_id], vps)) {
>>           ff_refstruct_unref(&vps);
>>       } else {
>>           remove_vps(ps, vps_id);
>> diff --git a/libavcodec/hevc_ps.h b/libavcodec/hevc_ps.h
>> index cc75aeb8d3..0d8eaf2b3e 100644
>> --- a/libavcodec/hevc_ps.h
>> +++ b/libavcodec/hevc_ps.h
>> @@ -153,7 +153,6 @@ typedef struct PTL {
>>
>>   typedef struct HEVCVPS {
>>       unsigned int vps_id;
>> -    HEVCHdrParams *hdr;
>>
>>       uint8_t vps_temporal_id_nesting_flag;
>>       int vps_max_layers;
>> @@ -175,6 +174,9 @@ typedef struct HEVCVPS {
>>
>>       uint8_t data[4096];
>>       int data_size;
>> +    /* Put this at the end of the structure to make it easier to calculate the
>> +     * size before this pointer, which is used for memcmp */
>> +    HEVCHdrParams *hdr;
>>   } HEVCVPS;
>>
>>   typedef struct ScalingList {
>_______________________________________________
>ffmpeg-devel mailing list
>ffmpeg-devel@ffmpeg.org
>https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>To unsubscribe, visit link above, or email
>ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Mark Thompson]

On 28/03/2024 13:15, tong1.wu-at-intel.com@ffmpeg.org wrote:
> From: Tong Wu <tong1.wu@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@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 problem is mostly harmless because the nondeterministic replacement of structs which we were expecting to be equivalent doesn't actually change anything, so why don't we just remove the comparison and always replace?

Thanks,

- Mark
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[James Almer]

On 3/29/2024 10:10 AM, Mark Thompson wrote:
> On 28/03/2024 13:15, tong1.wu-at-intel.com@ffmpeg.org wrote:
>> From: Tong Wu <tong1.wu@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@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?

> 
> The problem is mostly harmless because the nondeterministic replacement 
> of structs which we were expecting to be equivalent doesn't actually 
> change anything, so why don't we just remove the comparison and always 
> replace?
> 
> Thanks,
> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Andreas Rheinhardt]

James Almer:
> On 3/29/2024 10:10 AM, Mark Thompson wrote:
>> On 28/03/2024 13:15, tong1.wu-at-intel.com@ffmpeg.org wrote:
>>> From: Tong Wu <tong1.wu@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@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.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [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@ffmpeg.org wrote:
>>>> From: Tong Wu <tong1.wu@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@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Andreas Rheinhardt]

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@ffmpeg.org wrote:
>>>>> From: Tong Wu <tong1.wu@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@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.
This does not change that I consider it crazy to remove the parameter
sets referencing a parameter set that is removed.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [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@ffmpeg.org wrote:
>>>>>> From: Tong Wu <tong1.wu@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@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Anton Khirnov]

Quoting Andreas Rheinhardt (2024-03-29 16:58:48)
> This does not change that I consider it crazy to remove the parameter
> sets referencing a parameter set that is removed.

What's crazy about it? A PPS is parsed for a given SPS. If the SPS is
gone, then the PPS must be either removed or re-parsed with the new SPS.
The latter would be far more complex and AFAIK is not actually needed
for anything.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Andreas Rheinhardt]

Mark Thompson:
> On 28/03/2024 13:15, tong1.wu-at-intel.com@ffmpeg.org wrote:
>> From: Tong Wu <tong1.wu@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@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 problem is mostly harmless because the nondeterministic replacement
> of structs which we were expecting to be equivalent doesn't actually
> change anything, so why don't we just remove the comparison and always
> replace?
> 

remove_vps() also removes any SPS referencing this VPS (and remove_sps()
does the same with PPS). Therefore if you simply repeat a VPS without
also repeating the other parameter sets directly after the new VPS and
before the first video NALU after the VPS, your extradata will have been
discarded.
This is not what the spec says.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Wu, Tong1]

>From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>Andreas Rheinhardt
>Sent: Friday, March 29, 2024 10:03 PM
>To: ffmpeg-devel@ffmpeg.org
>Subject: Re: [FFmpeg-devel] [PATCH] avcodec/hevc_ps: fix the problem of
>memcmp losing effectiveness
>
>Mark Thompson:
>> On 28/03/2024 13:15, tong1.wu-at-intel.com@ffmpeg.org wrote:
>>> From: Tong Wu <tong1.wu@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@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 problem is mostly harmless because the nondeterministic replacement
>> of structs which we were expecting to be equivalent doesn't actually
>> change anything, so why don't we just remove the comparison and always
>> replace?
>>
>
>remove_vps() also removes any SPS referencing this VPS (and remove_sps()
>does the same with PPS). Therefore if you simply repeat a VPS without
>also repeating the other parameter sets directly after the new VPS and
>before the first video NALU after the VPS, your extradata will have been
>discarded.
>This is not what the spec says.
>
>

Yes and I observed for hevc decoder with hwaccel, get_format() is called multiple times which initializes the hwaccel context multiple times, as s->ps.sps is unexpectedly removed because of that.

Hendrik also observed some playback glitches(see previous email) so it's not really harmless.


>_______________________________________________
>ffmpeg-devel mailing list
>ffmpeg-devel@ffmpeg.org
>https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>To unsubscribe, visit link above, or email
>ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[tong1.wu-at-intel.com]

From: Tong Wu <tong1.wu@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@intel.com>
---
 libavcodec/hevc_ps.c | 20 ++++++++++++++++----
 libavcodec/hevc_ps.h |  2 +-
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
index cbef3ef4cd..8b3a27a17c 100644
--- a/libavcodec/hevc_ps.c
+++ b/libavcodec/hevc_ps.c
@@ -449,6 +449,16 @@ static void uninit_vps(FFRefStructOpaque opaque, void *obj)
     av_freep(&vps->hdr);
 }
 
+static int compare_vps(const HEVCVPS *vps1, const HEVCVPS *vps2)
+{
+    if ((!vps1->hdr && !vps2->hdr) ||
+        (vps1->hdr && vps2->hdr && !memcmp(vps1->hdr, vps2->hdr, sizeof(*vps1->hdr)))) {
+        return !memcmp(vps1, vps2, offsetof(HEVCVPS, hdr));
+    }
+
+    return 0;
+}
+
 int ff_hevc_decode_nal_vps(GetBitContext *gb, AVCodecContext *avctx,
                            HEVCParamSets *ps)
 {
@@ -545,9 +555,11 @@ int ff_hevc_decode_nal_vps(GetBitContext *gb, AVCodecContext *avctx,
             goto err;
         }
 
-        vps->hdr = av_calloc(vps->vps_num_hrd_parameters, sizeof(*vps->hdr));
-        if (!vps->hdr)
-            goto err;
+        if (vps->vps_num_hrd_parameters) {
+            vps->hdr = av_calloc(vps->vps_num_hrd_parameters, sizeof(*vps->hdr));
+            if (!vps->hdr)
+                goto err;
+        }
 
         for (i = 0; i < vps->vps_num_hrd_parameters; i++) {
             int common_inf_present = 1;
@@ -569,7 +581,7 @@ int ff_hevc_decode_nal_vps(GetBitContext *gb, AVCodecContext *avctx,
     }
 
     if (ps->vps_list[vps_id] &&
-        !memcmp(ps->vps_list[vps_id], vps, sizeof(*vps))) {
+        compare_vps(ps->vps_list[vps_id], vps)) {
         ff_refstruct_unref(&vps);
     } else {
         remove_vps(ps, vps_id);
diff --git a/libavcodec/hevc_ps.h b/libavcodec/hevc_ps.h
index cc75aeb8d3..8edac2508d 100644
--- a/libavcodec/hevc_ps.h
+++ b/libavcodec/hevc_ps.h
@@ -153,7 +153,6 @@ typedef struct PTL {
 
 typedef struct HEVCVPS {
     unsigned int vps_id;
-    HEVCHdrParams *hdr;
 
     uint8_t vps_temporal_id_nesting_flag;
     int vps_max_layers;
@@ -175,6 +174,7 @@ typedef struct HEVCVPS {
 
     uint8_t data[4096];
     int data_size;
+    HEVCHdrParams *hdr;
 } HEVCVPS;
 
 typedef struct ScalingList {
-- 
2.41.0.windows.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Hendrik Leppkes]

On Thu, Mar 28, 2024 at 10:12 AM <tong1.wu-at-intel.com@ffmpeg.org> wrote:
>
> From: Tong Wu <tong1.wu@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@intel.com>

I've been looking into some playback glitches after
456c8ebe7c7dcd766d36cd0296815d89fd1166b5 and I can confirm that this
patch fixes those as well.

Using the fixed position of the *hdr member and its offset seems a bit
icky though, and _at least_ could use a comment so future changes keep
it last.

- Hendrik
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Wu, Tong1]

>From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>Hendrik Leppkes
>Sent: Thursday, March 28, 2024 5:43 PM
>To: FFmpeg development discussions and patches <ffmpeg-
>devel@ffmpeg.org>
>Subject: Re: [FFmpeg-devel] [PATCH] avcodec/hevc_ps: fix the problem of
>memcmp losing effectiveness
>
>On Thu, Mar 28, 2024 at 10:12 AM <tong1.wu-at-intel.com@ffmpeg.org>
>wrote:
>>
>> From: Tong Wu <tong1.wu@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@intel.com>
>
>I've been looking into some playback glitches after
>456c8ebe7c7dcd766d36cd0296815d89fd1166b5 and I can confirm that this
>patch fixes those as well.
>
>Using the fixed position of the *hdr member and its offset seems a bit
>icky though, and _at least_ could use a comment so future changes keep
>it last.

Comment is added to the header.

Thanks,
Tong
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".