diff mbox series

[FFmpeg-devel] avcodec/cbs_h2645: keep separate parameter set lists for reading and writing

Message ID 20200607132737.1375-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel] avcodec/cbs_h2645: keep separate parameter set lists for reading and writing
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

James Almer June 7, 2020, 1:27 p.m. UTC
Similar logic as 4e2bef6a82. In scearios where an Access Unit is written right
after reading it using the same CBS context (hevc_metadata), the list of parsed
parameters sets used by the writer must not be the one that's the result of the
reader having already parsed the current Access Unit in question.

Fixes: out of array access
Fixes: 23034/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5074645169733632.fuzz

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: James Almer <jamrial@gmail.com>
---
An alternative is forcing the usage of separate CBS contexts for reading and
writing.

 libavcodec/cbs_h264.h  | 18 ++++++++---
 libavcodec/cbs_h2645.c | 72 ++++++++++++++++++++++++++++++++----------
 libavcodec/cbs_h265.h  | 26 +++++++++++----
 3 files changed, 89 insertions(+), 27 deletions(-)

Comments

Michael Niedermayer June 7, 2020, 2:45 p.m. UTC | #1
On Sun, Jun 07, 2020 at 10:27:37AM -0300, James Almer wrote:
> Similar logic as 4e2bef6a82. In scearios where an Access Unit is written right
> after reading it using the same CBS context (hevc_metadata), the list of parsed
> parameters sets used by the writer must not be the one that's the result of the
> reader having already parsed the current Access Unit in question.
> 
> Fixes: out of array access
> Fixes: 23034/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5074645169733632.fuzz
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> An alternative is forcing the usage of separate CBS contexts for reading and
> writing.
> 
>  libavcodec/cbs_h264.h  | 18 ++++++++---
>  libavcodec/cbs_h2645.c | 72 ++++++++++++++++++++++++++++++++----------
>  libavcodec/cbs_h265.h  | 26 +++++++++++----
>  3 files changed, 89 insertions(+), 27 deletions(-)

i think the change is probably ok and it fixes the issue
what i feel uneasy about is if this is the sole way the security
issue is fixed.

let me try to explain what i mean by a simpler example:
if you have a sprintf() that overwrites the buffer there are 3 ways
to fix that
A. You make the buffer big enough for what is written
B. You make the amount written only as large as the buffer
C. You check by using snprintf()

Now like here A/B may represent a bugfix
the problem with A/B is that security rests on potentially complex code
So even when A/B is done, we also should do C

This patch fixes the inconsistency on the write side be keeping more references
to the parameter sets.
For security one would have to proof that no crafted input to the reader
fed into any available useer of the reader could result in an inconsistency
to a writer following.
Are you sure thats the case now and with future users of the code ?
OTOH as dumb as a check in the writer may look, anyone can proof it fixes the
specific inconsistency.

What i suggest specifically is to also include or apply the simple check
on top of this, or a equivalent / more generic check. So that security does not
rest on alot of spread out code

Thanks

[...]
James Almer June 7, 2020, 3:20 p.m. UTC | #2
On 6/7/2020 11:45 AM, Michael Niedermayer wrote:
> On Sun, Jun 07, 2020 at 10:27:37AM -0300, James Almer wrote:
>> Similar logic as 4e2bef6a82. In scearios where an Access Unit is written right
>> after reading it using the same CBS context (hevc_metadata), the list of parsed
>> parameters sets used by the writer must not be the one that's the result of the
>> reader having already parsed the current Access Unit in question.
>>
>> Fixes: out of array access
>> Fixes: 23034/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5074645169733632.fuzz
>>
>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> An alternative is forcing the usage of separate CBS contexts for reading and
>> writing.
>>
>>  libavcodec/cbs_h264.h  | 18 ++++++++---
>>  libavcodec/cbs_h2645.c | 72 ++++++++++++++++++++++++++++++++----------
>>  libavcodec/cbs_h265.h  | 26 +++++++++++----
>>  3 files changed, 89 insertions(+), 27 deletions(-)
> 
> i think the change is probably ok and it fixes the issue
> what i feel uneasy about is if this is the sole way the security
> issue is fixed.
> 
> let me try to explain what i mean by a simpler example:
> if you have a sprintf() that overwrites the buffer there are 3 ways
> to fix that
> A. You make the buffer big enough for what is written
> B. You make the amount written only as large as the buffer
> C. You check by using snprintf()
> 
> Now like here A/B may represent a bugfix
> the problem with A/B is that security rests on potentially complex code
> So even when A/B is done, we also should do C
> 
> This patch fixes the inconsistency on the write side be keeping more references
> to the parameter sets.
> For security one would have to proof that no crafted input to the reader
> fed into any available useer of the reader could result in an inconsistency
> to a writer following.
> Are you sure thats the case now and with future users of the code ?
> OTOH as dumb as a check in the writer may look, anyone can proof it fixes the
> specific inconsistency.
> 
> What i suggest specifically is to also include or apply the simple check
> on top of this, or a equivalent / more generic check. So that security does not
> rest on alot of spread out code
> 
> Thanks

Well, one possibility is that after this, the infer() warning could be
replaced with an assert() instead. The CBS framework is not public, so
crashing with an assert() would be acceptable as infer() failing in
writing scenarios should be considered an internal bug (bitstream
filters, or any CBS user for that matter, should ensure to not change
fields in a way that would result in an invalid bitstream and thus
failing infer() checks).

The issue shouldn't be treated as "If inter_ref_pic_set_prediction_flag
is 1 in this scenario, then we should stop to avoid out of array
access", but as "We did something wrong because
inter_ref_pic_set_prediction_flag was absolutely not meant to be 1 at
this point". So using assert() after this patch sounds like a good
solution and will help detect future bugs in the parsing code.
James Almer June 7, 2020, 3:45 p.m. UTC | #3
On 6/7/2020 12:20 PM, James Almer wrote:
> On 6/7/2020 11:45 AM, Michael Niedermayer wrote:
>> On Sun, Jun 07, 2020 at 10:27:37AM -0300, James Almer wrote:
>>> Similar logic as 4e2bef6a82. In scearios where an Access Unit is written right
>>> after reading it using the same CBS context (hevc_metadata), the list of parsed
>>> parameters sets used by the writer must not be the one that's the result of the
>>> reader having already parsed the current Access Unit in question.
>>>
>>> Fixes: out of array access
>>> Fixes: 23034/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5074645169733632.fuzz
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>> An alternative is forcing the usage of separate CBS contexts for reading and
>>> writing.
>>>
>>>  libavcodec/cbs_h264.h  | 18 ++++++++---
>>>  libavcodec/cbs_h2645.c | 72 ++++++++++++++++++++++++++++++++----------
>>>  libavcodec/cbs_h265.h  | 26 +++++++++++----
>>>  3 files changed, 89 insertions(+), 27 deletions(-)
>>
>> i think the change is probably ok and it fixes the issue
>> what i feel uneasy about is if this is the sole way the security
>> issue is fixed.
>>
>> let me try to explain what i mean by a simpler example:
>> if you have a sprintf() that overwrites the buffer there are 3 ways
>> to fix that
>> A. You make the buffer big enough for what is written
>> B. You make the amount written only as large as the buffer
>> C. You check by using snprintf()
>>
>> Now like here A/B may represent a bugfix
>> the problem with A/B is that security rests on potentially complex code
>> So even when A/B is done, we also should do C
>>
>> This patch fixes the inconsistency on the write side be keeping more references
>> to the parameter sets.
>> For security one would have to proof that no crafted input to the reader
>> fed into any available useer of the reader could result in an inconsistency
>> to a writer following.
>> Are you sure thats the case now and with future users of the code ?
>> OTOH as dumb as a check in the writer may look, anyone can proof it fixes the
>> specific inconsistency.
>>
>> What i suggest specifically is to also include or apply the simple check
>> on top of this, or a equivalent / more generic check. So that security does not
>> rest on alot of spread out code
>>
>> Thanks
> 
> Well, one possibility is that after this, the infer() warning could be
> replaced with an assert() instead. The CBS framework is not public, so
> crashing with an assert() would be acceptable as infer() failing in
> writing scenarios should be considered an internal bug (bitstream
> filters, or any CBS user for that matter, should ensure to not change
> fields in a way that would result in an invalid bitstream and thus
> failing infer() checks).
> 
> The issue shouldn't be treated as "If inter_ref_pic_set_prediction_flag
> is 1 in this scenario, then we should stop to avoid out of array
> access", but as "We did something wrong because
> inter_ref_pic_set_prediction_flag was absolutely not meant to be 1 at
> this point". So using assert() after this patch sounds like a good
> solution and will help detect future bugs in the parsing code.

It could also be a generic return AVERROR_INVALIDDATA as you suggested,
to be fair. All the standard writing helpers abort gracefully that way,
so infer() could do the same.

Or the other helpers could be changed to assert().
Michael Niedermayer July 1, 2020, 9:40 a.m. UTC | #4
On Sun, Jun 07, 2020 at 12:45:15PM -0300, James Almer wrote:
> On 6/7/2020 12:20 PM, James Almer wrote:
> > On 6/7/2020 11:45 AM, Michael Niedermayer wrote:
> >> On Sun, Jun 07, 2020 at 10:27:37AM -0300, James Almer wrote:
> >>> Similar logic as 4e2bef6a82. In scearios where an Access Unit is written right
> >>> after reading it using the same CBS context (hevc_metadata), the list of parsed
> >>> parameters sets used by the writer must not be the one that's the result of the
> >>> reader having already parsed the current Access Unit in question.
> >>>
> >>> Fixes: out of array access
> >>> Fixes: 23034/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5074645169733632.fuzz
> >>>
> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>> Signed-off-by: James Almer <jamrial@gmail.com>
> >>> ---
> >>> An alternative is forcing the usage of separate CBS contexts for reading and
> >>> writing.
> >>>
> >>>  libavcodec/cbs_h264.h  | 18 ++++++++---
> >>>  libavcodec/cbs_h2645.c | 72 ++++++++++++++++++++++++++++++++----------
> >>>  libavcodec/cbs_h265.h  | 26 +++++++++++----
> >>>  3 files changed, 89 insertions(+), 27 deletions(-)
> >>
> >> i think the change is probably ok and it fixes the issue
> >> what i feel uneasy about is if this is the sole way the security
> >> issue is fixed.
> >>
> >> let me try to explain what i mean by a simpler example:
> >> if you have a sprintf() that overwrites the buffer there are 3 ways
> >> to fix that
> >> A. You make the buffer big enough for what is written
> >> B. You make the amount written only as large as the buffer
> >> C. You check by using snprintf()
> >>
> >> Now like here A/B may represent a bugfix
> >> the problem with A/B is that security rests on potentially complex code
> >> So even when A/B is done, we also should do C
> >>
> >> This patch fixes the inconsistency on the write side be keeping more references
> >> to the parameter sets.
> >> For security one would have to proof that no crafted input to the reader
> >> fed into any available useer of the reader could result in an inconsistency
> >> to a writer following.
> >> Are you sure thats the case now and with future users of the code ?
> >> OTOH as dumb as a check in the writer may look, anyone can proof it fixes the
> >> specific inconsistency.
> >>
> >> What i suggest specifically is to also include or apply the simple check
> >> on top of this, or a equivalent / more generic check. So that security does not
> >> rest on alot of spread out code
> >>
> >> Thanks
> > 
> > Well, one possibility is that after this, the infer() warning could be
> > replaced with an assert() instead. The CBS framework is not public, so
> > crashing with an assert() would be acceptable as infer() failing in
> > writing scenarios should be considered an internal bug (bitstream
> > filters, or any CBS user for that matter, should ensure to not change
> > fields in a way that would result in an invalid bitstream and thus
> > failing infer() checks).
> > 
> > The issue shouldn't be treated as "If inter_ref_pic_set_prediction_flag
> > is 1 in this scenario, then we should stop to avoid out of array
> > access", but as "We did something wrong because
> > inter_ref_pic_set_prediction_flag was absolutely not meant to be 1 at
> > this point". So using assert() after this patch sounds like a good
> > solution and will help detect future bugs in the parsing code.
> 
> It could also be a generic return AVERROR_INVALIDDATA as you suggested,
> to be fair. All the standard writing helpers abort gracefully that way,
> so infer() could do the same.
> 
> Or the other helpers could be changed to assert().

whats the status of this ?
has this issue been fixed in some other way i missed ?
will this get applied ?
should i apply my not so pretty fix for 23034 ?
what will be done about releases ? can this be backported ?

PS: please just make sure 23034/ is mentioned in the commit message so
whatever fixes it can be kept track of (i know its already mentioned
this is more intended as a remainder for other alternative fixes)

thx

[...]
Andreas Rheinhardt July 1, 2020, 11:59 a.m. UTC | #5
James Almer:
> Similar logic as 4e2bef6a82. In scearios where an Access Unit is written right
> after reading it using the same CBS context (hevc_metadata), the list of parsed
> parameters sets used by the writer must not be the one that's the result of the
> reader having already parsed the current Access Unit in question.
> 
> Fixes: out of array access
> Fixes: 23034/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5074645169733632.fuzz
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> An alternative is forcing the usage of separate CBS contexts for reading and
> writing.
> 

Am I the only one who thinks this would be cleaner, as it would directly
show to the user that there are two separate states involved? It would
also lead to a smaller diff.

- Andreas
James Almer July 1, 2020, 1:22 p.m. UTC | #6
On 7/1/2020 6:40 AM, Michael Niedermayer wrote:
> On Sun, Jun 07, 2020 at 12:45:15PM -0300, James Almer wrote:
>> On 6/7/2020 12:20 PM, James Almer wrote:
>>> On 6/7/2020 11:45 AM, Michael Niedermayer wrote:
>>>> On Sun, Jun 07, 2020 at 10:27:37AM -0300, James Almer wrote:
>>>>> Similar logic as 4e2bef6a82. In scearios where an Access Unit is written right
>>>>> after reading it using the same CBS context (hevc_metadata), the list of parsed
>>>>> parameters sets used by the writer must not be the one that's the result of the
>>>>> reader having already parsed the current Access Unit in question.
>>>>>
>>>>> Fixes: out of array access
>>>>> Fixes: 23034/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5074645169733632.fuzz
>>>>>
>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>> ---
>>>>> An alternative is forcing the usage of separate CBS contexts for reading and
>>>>> writing.
>>>>>
>>>>>  libavcodec/cbs_h264.h  | 18 ++++++++---
>>>>>  libavcodec/cbs_h2645.c | 72 ++++++++++++++++++++++++++++++++----------
>>>>>  libavcodec/cbs_h265.h  | 26 +++++++++++----
>>>>>  3 files changed, 89 insertions(+), 27 deletions(-)
>>>>
>>>> i think the change is probably ok and it fixes the issue
>>>> what i feel uneasy about is if this is the sole way the security
>>>> issue is fixed.
>>>>
>>>> let me try to explain what i mean by a simpler example:
>>>> if you have a sprintf() that overwrites the buffer there are 3 ways
>>>> to fix that
>>>> A. You make the buffer big enough for what is written
>>>> B. You make the amount written only as large as the buffer
>>>> C. You check by using snprintf()
>>>>
>>>> Now like here A/B may represent a bugfix
>>>> the problem with A/B is that security rests on potentially complex code
>>>> So even when A/B is done, we also should do C
>>>>
>>>> This patch fixes the inconsistency on the write side be keeping more references
>>>> to the parameter sets.
>>>> For security one would have to proof that no crafted input to the reader
>>>> fed into any available useer of the reader could result in an inconsistency
>>>> to a writer following.
>>>> Are you sure thats the case now and with future users of the code ?
>>>> OTOH as dumb as a check in the writer may look, anyone can proof it fixes the
>>>> specific inconsistency.
>>>>
>>>> What i suggest specifically is to also include or apply the simple check
>>>> on top of this, or a equivalent / more generic check. So that security does not
>>>> rest on alot of spread out code
>>>>
>>>> Thanks
>>>
>>> Well, one possibility is that after this, the infer() warning could be
>>> replaced with an assert() instead. The CBS framework is not public, so
>>> crashing with an assert() would be acceptable as infer() failing in
>>> writing scenarios should be considered an internal bug (bitstream
>>> filters, or any CBS user for that matter, should ensure to not change
>>> fields in a way that would result in an invalid bitstream and thus
>>> failing infer() checks).
>>>
>>> The issue shouldn't be treated as "If inter_ref_pic_set_prediction_flag
>>> is 1 in this scenario, then we should stop to avoid out of array
>>> access", but as "We did something wrong because
>>> inter_ref_pic_set_prediction_flag was absolutely not meant to be 1 at
>>> this point". So using assert() after this patch sounds like a good
>>> solution and will help detect future bugs in the parsing code.
>>
>> It could also be a generic return AVERROR_INVALIDDATA as you suggested,
>> to be fair. All the standard writing helpers abort gracefully that way,
>> so infer() could do the same.
>>
>> Or the other helpers could be changed to assert().
> 
> whats the status of this ?
> has this issue been fixed in some other way i missed ?

I pushed the first two patches and backported them, so the issue should
be fixed by the first.

> will this get applied ?

I delayed applying this one waiting for more opinions, especially
Mark's, since it's kind of ugly.

> should i apply my not so pretty fix for 23034 ?
> what will be done about releases ? can this be backported ?

Already answered above, but maybe confirm it's fixed just to be sure.

> 
> PS: please just make sure 23034/ is mentioned in the commit message so
> whatever fixes it can be kept track of (i know its already mentioned
> this is more intended as a remainder for other alternative fixes)

Ah, guess i should have mentioned that ossfuzz issue in ef13fafe22 (and
e6ab99f324by extension) seeing i didn't push this one alongside it...

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> 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".
>
Michael Niedermayer July 2, 2020, 12:12 p.m. UTC | #7
On Wed, Jul 01, 2020 at 10:22:48AM -0300, James Almer wrote:
> On 7/1/2020 6:40 AM, Michael Niedermayer wrote:
> > On Sun, Jun 07, 2020 at 12:45:15PM -0300, James Almer wrote:
> >> On 6/7/2020 12:20 PM, James Almer wrote:
> >>> On 6/7/2020 11:45 AM, Michael Niedermayer wrote:
> >>>> On Sun, Jun 07, 2020 at 10:27:37AM -0300, James Almer wrote:
> >>>>> Similar logic as 4e2bef6a82. In scearios where an Access Unit is written right
> >>>>> after reading it using the same CBS context (hevc_metadata), the list of parsed
> >>>>> parameters sets used by the writer must not be the one that's the result of the
> >>>>> reader having already parsed the current Access Unit in question.
> >>>>>
> >>>>> Fixes: out of array access
> >>>>> Fixes: 23034/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5074645169733632.fuzz
> >>>>>
> >>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>>>> Signed-off-by: James Almer <jamrial@gmail.com>
> >>>>> ---
> >>>>> An alternative is forcing the usage of separate CBS contexts for reading and
> >>>>> writing.
> >>>>>
> >>>>>  libavcodec/cbs_h264.h  | 18 ++++++++---
> >>>>>  libavcodec/cbs_h2645.c | 72 ++++++++++++++++++++++++++++++++----------
> >>>>>  libavcodec/cbs_h265.h  | 26 +++++++++++----
> >>>>>  3 files changed, 89 insertions(+), 27 deletions(-)
> >>>>
> >>>> i think the change is probably ok and it fixes the issue
> >>>> what i feel uneasy about is if this is the sole way the security
> >>>> issue is fixed.
> >>>>
> >>>> let me try to explain what i mean by a simpler example:
> >>>> if you have a sprintf() that overwrites the buffer there are 3 ways
> >>>> to fix that
> >>>> A. You make the buffer big enough for what is written
> >>>> B. You make the amount written only as large as the buffer
> >>>> C. You check by using snprintf()
> >>>>
> >>>> Now like here A/B may represent a bugfix
> >>>> the problem with A/B is that security rests on potentially complex code
> >>>> So even when A/B is done, we also should do C
> >>>>
> >>>> This patch fixes the inconsistency on the write side be keeping more references
> >>>> to the parameter sets.
> >>>> For security one would have to proof that no crafted input to the reader
> >>>> fed into any available useer of the reader could result in an inconsistency
> >>>> to a writer following.
> >>>> Are you sure thats the case now and with future users of the code ?
> >>>> OTOH as dumb as a check in the writer may look, anyone can proof it fixes the
> >>>> specific inconsistency.
> >>>>
> >>>> What i suggest specifically is to also include or apply the simple check
> >>>> on top of this, or a equivalent / more generic check. So that security does not
> >>>> rest on alot of spread out code
> >>>>
> >>>> Thanks
> >>>
> >>> Well, one possibility is that after this, the infer() warning could be
> >>> replaced with an assert() instead. The CBS framework is not public, so
> >>> crashing with an assert() would be acceptable as infer() failing in
> >>> writing scenarios should be considered an internal bug (bitstream
> >>> filters, or any CBS user for that matter, should ensure to not change
> >>> fields in a way that would result in an invalid bitstream and thus
> >>> failing infer() checks).
> >>>
> >>> The issue shouldn't be treated as "If inter_ref_pic_set_prediction_flag
> >>> is 1 in this scenario, then we should stop to avoid out of array
> >>> access", but as "We did something wrong because
> >>> inter_ref_pic_set_prediction_flag was absolutely not meant to be 1 at
> >>> this point". So using assert() after this patch sounds like a good
> >>> solution and will help detect future bugs in the parsing code.
> >>
> >> It could also be a generic return AVERROR_INVALIDDATA as you suggested,
> >> to be fair. All the standard writing helpers abort gracefully that way,
> >> so infer() could do the same.
> >>
> >> Or the other helpers could be changed to assert().
> > 
> > whats the status of this ?
> > has this issue been fixed in some other way i missed ?
> 
> I pushed the first two patches and backported them, so the issue should
> be fixed by the first.
> 
> > will this get applied ?
> 
> I delayed applying this one waiting for more opinions, especially
> Mark's, since it's kind of ugly.
> 
> > should i apply my not so pretty fix for 23034 ?
> > what will be done about releases ? can this be backported ?
> 
> Already answered above, but maybe confirm it's fixed just to be sure.

23034 no longer reproduces on master locally

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/cbs_h264.h b/libavcodec/cbs_h264.h
index 9f7c2a0d30..9d104787d9 100644
--- a/libavcodec/cbs_h264.h
+++ b/libavcodec/cbs_h264.h
@@ -448,10 +448,20 @@  typedef struct CodedBitstreamH264Context {
 
     // All currently available parameter sets.  These are updated when
     // any parameter set NAL unit is read/written with this context.
-    AVBufferRef *sps_ref[H264_MAX_SPS_COUNT];
-    AVBufferRef *pps_ref[H264_MAX_PPS_COUNT];
-    H264RawSPS *sps[H264_MAX_SPS_COUNT];
-    H264RawPPS *pps[H264_MAX_PPS_COUNT];
+    AVBufferRef **sps_ref;
+    AVBufferRef **pps_ref;
+    H264RawSPS **sps;
+    H264RawPPS **pps;
+
+    AVBufferRef *read_sps_ref[H264_MAX_SPS_COUNT];
+    AVBufferRef *read_pps_ref[H264_MAX_PPS_COUNT];
+    H264RawSPS *read_sps[H264_MAX_SPS_COUNT];
+    H264RawPPS *read_pps[H264_MAX_PPS_COUNT];
+
+    AVBufferRef *write_sps_ref[H264_MAX_SPS_COUNT];
+    AVBufferRef *write_pps_ref[H264_MAX_PPS_COUNT];
+    H264RawSPS *write_sps[H264_MAX_SPS_COUNT];
+    H264RawPPS *write_pps[H264_MAX_PPS_COUNT];
 
     // The currently active parameter sets.  These are updated when any
     // NAL unit refers to the relevant parameter set.  These pointers
diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
index b432921ecc..69ed890c63 100644
--- a/libavcodec/cbs_h2645.c
+++ b/libavcodec/cbs_h2645.c
@@ -758,14 +758,14 @@  static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx,
     return 0;
 }
 
-#define cbs_h2645_replace_ps(h26n, ps_name, ps_var, id_element) \
+#define cbs_h2645_replace_ps(h26n, Hn, ps_name, ps_var, id_element) \
 static int cbs_h26 ## h26n ## _replace_ ## ps_var(CodedBitstreamContext *ctx, \
                                                   CodedBitstreamUnit *unit)  \
 { \
     CodedBitstreamH26 ## h26n ## Context *priv = ctx->priv_data; \
     H26 ## h26n ## Raw ## ps_name *ps_var = unit->content; \
     unsigned int id = ps_var->id_element; \
-    if (id >= FF_ARRAY_ELEMS(priv->ps_var)) { \
+    if (id >= Hn ## _MAX_ ## ps_name ## _COUNT) { \
         av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid " #ps_name \
                " id : %d.\n", id); \
         return AVERROR_INVALIDDATA; \
@@ -785,18 +785,24 @@  static int cbs_h26 ## h26n ## _replace_ ## ps_var(CodedBitstreamContext *ctx, \
     return 0; \
 }
 
-cbs_h2645_replace_ps(4, SPS, sps, seq_parameter_set_id)
-cbs_h2645_replace_ps(4, PPS, pps, pic_parameter_set_id)
-cbs_h2645_replace_ps(5, VPS, vps, vps_video_parameter_set_id)
-cbs_h2645_replace_ps(5, SPS, sps, sps_seq_parameter_set_id)
-cbs_h2645_replace_ps(5, PPS, pps, pps_pic_parameter_set_id)
+cbs_h2645_replace_ps(4, H264, SPS, sps, seq_parameter_set_id)
+cbs_h2645_replace_ps(4, H264, PPS, pps, pic_parameter_set_id)
+cbs_h2645_replace_ps(5, HEVC, VPS, vps, vps_video_parameter_set_id)
+cbs_h2645_replace_ps(5, HEVC, SPS, sps, sps_seq_parameter_set_id)
+cbs_h2645_replace_ps(5, HEVC, PPS, pps, pps_pic_parameter_set_id)
 
 static int cbs_h264_read_nal_unit(CodedBitstreamContext *ctx,
                                   CodedBitstreamUnit *unit)
 {
+    CodedBitstreamH264Context *priv = ctx->priv_data;
     GetBitContext gbc;
     int err;
 
+    priv->sps_ref = (AVBufferRef **)priv->read_sps_ref;
+    priv->pps_ref = (AVBufferRef **)priv->read_pps_ref;
+    priv->sps = (H264RawSPS **)priv->read_sps;
+    priv->pps = (H264RawPPS **)priv->read_pps;
+
     err = init_get_bits(&gbc, unit->data, 8 * unit->data_size);
     if (err < 0)
         return err;
@@ -953,9 +959,17 @@  static int cbs_h264_read_nal_unit(CodedBitstreamContext *ctx,
 static int cbs_h265_read_nal_unit(CodedBitstreamContext *ctx,
                                   CodedBitstreamUnit *unit)
 {
+    CodedBitstreamH265Context *priv = ctx->priv_data;
     GetBitContext gbc;
     int err;
 
+    priv->vps_ref = (AVBufferRef **)priv->read_vps_ref;
+    priv->sps_ref = (AVBufferRef **)priv->read_sps_ref;
+    priv->pps_ref = (AVBufferRef **)priv->read_pps_ref;
+    priv->vps = (H265RawVPS **)priv->read_vps;
+    priv->sps = (H265RawSPS **)priv->read_sps;
+    priv->pps = (H265RawPPS **)priv->read_pps;
+
     err = init_get_bits(&gbc, unit->data, 8 * unit->data_size);
     if (err < 0)
         return err;
@@ -1164,8 +1178,14 @@  static int cbs_h264_write_nal_unit(CodedBitstreamContext *ctx,
                                    CodedBitstreamUnit *unit,
                                    PutBitContext *pbc)
 {
+    CodedBitstreamH264Context *priv = ctx->priv_data;
     int err;
 
+    priv->sps_ref = (AVBufferRef **)priv->write_sps_ref;
+    priv->pps_ref = (AVBufferRef **)priv->write_pps_ref;
+    priv->sps = (H264RawSPS **)priv->write_sps;
+    priv->pps = (H264RawPPS **)priv->write_pps;
+
     switch (unit->type) {
     case H264_NAL_SPS:
         {
@@ -1281,8 +1301,16 @@  static int cbs_h265_write_nal_unit(CodedBitstreamContext *ctx,
                                    CodedBitstreamUnit *unit,
                                    PutBitContext *pbc)
 {
+    CodedBitstreamH265Context *priv = ctx->priv_data;
     int err;
 
+    priv->vps_ref = (AVBufferRef **)priv->write_vps_ref;
+    priv->sps_ref = (AVBufferRef **)priv->write_sps_ref;
+    priv->pps_ref = (AVBufferRef **)priv->write_pps_ref;
+    priv->vps = (H265RawVPS **)priv->write_vps;
+    priv->sps = (H265RawSPS **)priv->write_sps;
+    priv->pps = (H265RawPPS **)priv->write_pps;
+
     switch (unit->type) {
     case HEVC_NAL_VPS:
         {
@@ -1483,10 +1511,14 @@  static void cbs_h264_close(CodedBitstreamContext *ctx)
 
     ff_h2645_packet_uninit(&h264->common.read_packet);
 
-    for (i = 0; i < FF_ARRAY_ELEMS(h264->sps); i++)
-        av_buffer_unref(&h264->sps_ref[i]);
-    for (i = 0; i < FF_ARRAY_ELEMS(h264->pps); i++)
-        av_buffer_unref(&h264->pps_ref[i]);
+    for (i = 0; i < H264_MAX_SPS_COUNT; i++) {
+        av_buffer_unref(&h264->read_sps_ref[i]);
+        av_buffer_unref(&h264->write_sps_ref[i]);
+    }
+    for (i = 0; i < H264_MAX_PPS_COUNT; i++) {
+        av_buffer_unref(&h264->read_pps_ref[i]);
+        av_buffer_unref(&h264->write_pps_ref[i]);
+    }
 }
 
 static void cbs_h265_close(CodedBitstreamContext *ctx)
@@ -1496,12 +1528,18 @@  static void cbs_h265_close(CodedBitstreamContext *ctx)
 
     ff_h2645_packet_uninit(&h265->common.read_packet);
 
-    for (i = 0; i < FF_ARRAY_ELEMS(h265->vps); i++)
-        av_buffer_unref(&h265->vps_ref[i]);
-    for (i = 0; i < FF_ARRAY_ELEMS(h265->sps); i++)
-        av_buffer_unref(&h265->sps_ref[i]);
-    for (i = 0; i < FF_ARRAY_ELEMS(h265->pps); i++)
-        av_buffer_unref(&h265->pps_ref[i]);
+    for (i = 0; i < HEVC_MAX_VPS_COUNT; i++) {
+        av_buffer_unref(&h265->read_vps_ref[i]);
+        av_buffer_unref(&h265->write_vps_ref[i]);
+    }
+    for (i = 0; i < HEVC_MAX_SPS_COUNT; i++) {
+        av_buffer_unref(&h265->read_sps_ref[i]);
+        av_buffer_unref(&h265->write_sps_ref[i]);
+    }
+    for (i = 0; i < HEVC_MAX_PPS_COUNT; i++) {
+        av_buffer_unref(&h265->read_pps_ref[i]);
+        av_buffer_unref(&h265->write_pps_ref[i]);
+    }
 }
 
 const CodedBitstreamType ff_cbs_type_h264 = {
diff --git a/libavcodec/cbs_h265.h b/libavcodec/cbs_h265.h
index 73897f77a4..ab27f77f15 100644
--- a/libavcodec/cbs_h265.h
+++ b/libavcodec/cbs_h265.h
@@ -731,12 +731,26 @@  typedef struct CodedBitstreamH265Context {
 
     // All currently available parameter sets.  These are updated when
     // any parameter set NAL unit is read/written with this context.
-    AVBufferRef *vps_ref[HEVC_MAX_VPS_COUNT];
-    AVBufferRef *sps_ref[HEVC_MAX_SPS_COUNT];
-    AVBufferRef *pps_ref[HEVC_MAX_PPS_COUNT];
-    H265RawVPS *vps[HEVC_MAX_VPS_COUNT];
-    H265RawSPS *sps[HEVC_MAX_SPS_COUNT];
-    H265RawPPS *pps[HEVC_MAX_PPS_COUNT];
+    AVBufferRef **vps_ref;
+    AVBufferRef **sps_ref;
+    AVBufferRef **pps_ref;
+    H265RawVPS **vps;
+    H265RawSPS **sps;
+    H265RawPPS **pps;
+
+    AVBufferRef *read_vps_ref[HEVC_MAX_VPS_COUNT];
+    AVBufferRef *read_sps_ref[HEVC_MAX_SPS_COUNT];
+    AVBufferRef *read_pps_ref[HEVC_MAX_PPS_COUNT];
+    H265RawVPS *read_vps[HEVC_MAX_VPS_COUNT];
+    H265RawSPS *read_sps[HEVC_MAX_SPS_COUNT];
+    H265RawPPS *read_pps[HEVC_MAX_PPS_COUNT];
+
+    AVBufferRef *write_vps_ref[HEVC_MAX_VPS_COUNT];
+    AVBufferRef *write_sps_ref[HEVC_MAX_SPS_COUNT];
+    AVBufferRef *write_pps_ref[HEVC_MAX_PPS_COUNT];
+    H265RawVPS *write_vps[HEVC_MAX_VPS_COUNT];
+    H265RawSPS *write_sps[HEVC_MAX_SPS_COUNT];
+    H265RawPPS *write_pps[HEVC_MAX_PPS_COUNT];
 
     // The currently active parameter sets.  These are updated when any
     // NAL unit refers to the relevant parameter set.  These pointers