[FFmpeg-devel,v1] avcodec/h2645_parse: fix for the realloc size

Submitted by lance.lmwang@gmail.com on Sept. 10, 2019, 1:18 a.m.

Details

Message ID 1568078324-8655-1-git-send-email-lance.lmwang@gmail.com
State New
Headers show

Commit Message

lance.lmwang@gmail.com Sept. 10, 2019, 1:18 a.m.
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavcodec/h2645_parse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

James Almer Sept. 10, 2019, 1:53 a.m.
On 9/9/2019 10:18 PM, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavcodec/h2645_parse.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
> index 307e8643e6..403acd2ee1 100644
> --- a/libavcodec/h2645_parse.c
> +++ b/libavcodec/h2645_parse.c
> @@ -454,7 +454,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>          }
>  
>          if (pkt->nals_allocated < pkt->nb_nals + 1) {
> -            int new_size = pkt->nals_allocated + 1;
> +            int new_size = pkt->nb_nals + 1;

No. nals_allocated and nb_nals are two purposely separate values in
order to reuse the H2645Packet struct. One keeps track of the allocated
size of the NAL buffer, whereas the other keeps track of the amount of
NALs used by the current packet. That's why the check immediately above
this line makes sure the amount of NALs in the current packet fit in the
current size of the NAL buffer, and grows/reallocates it otherwise.

>              void *tmp = av_realloc_array(pkt->nals, new_size, sizeof(*pkt->nals));
>  
>              if (!tmp)
>
lance.lmwang@gmail.com Sept. 10, 2019, 2:51 a.m.
On Mon, Sep 09, 2019 at 10:53:53PM -0300, James Almer wrote:
> On 9/9/2019 10:18 PM, lance.lmwang@gmail.com wrote:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavcodec/h2645_parse.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
> > index 307e8643e6..403acd2ee1 100644
> > --- a/libavcodec/h2645_parse.c
> > +++ b/libavcodec/h2645_parse.c
> > @@ -454,7 +454,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
> >          }
> >  
> >          if (pkt->nals_allocated < pkt->nb_nals + 1) {
> > -            int new_size = pkt->nals_allocated + 1;
> > +            int new_size = pkt->nb_nals + 1;
> 
> No. nals_allocated and nb_nals are two purposely separate values in
> order to reuse the H2645Packet struct. One keeps track of the allocated
> size of the NAL buffer, whereas the other keeps track of the amount of
> NALs used by the current packet. That's why the check immediately above
> this line makes sure the amount of NALs in the current packet fit in the
> current size of the NAL buffer, and grows/reallocates it otherwise.

Thanks for the detail information about the two field. Below is my assumation 
as I haven't such sample to test such case.
Assuming nals_allocated is 1, nb_nals is 3, then the current code will reallocate
new_size with 2, But the following code will use the nb_nals to access pkt->nals, 
I'm not sure nals memory is allocated or not?
nal = &pkt->nals[pkt->nb_nals];


> 
> >              void *tmp = av_realloc_array(pkt->nals, new_size, sizeof(*pkt->nals));
> >  
> >              if (!tmp)
> > 
> 
> _______________________________________________
> 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".
James Almer Sept. 10, 2019, 3:03 a.m.
On 9/9/2019 11:51 PM, Limin Wang wrote:
> On Mon, Sep 09, 2019 at 10:53:53PM -0300, James Almer wrote:
>> On 9/9/2019 10:18 PM, lance.lmwang@gmail.com wrote:
>>> From: Limin Wang <lance.lmwang@gmail.com>
>>>
>>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>>> ---
>>>  libavcodec/h2645_parse.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
>>> index 307e8643e6..403acd2ee1 100644
>>> --- a/libavcodec/h2645_parse.c
>>> +++ b/libavcodec/h2645_parse.c
>>> @@ -454,7 +454,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>>>          }
>>>  
>>>          if (pkt->nals_allocated < pkt->nb_nals + 1) {
>>> -            int new_size = pkt->nals_allocated + 1;
>>> +            int new_size = pkt->nb_nals + 1;
>>
>> No. nals_allocated and nb_nals are two purposely separate values in
>> order to reuse the H2645Packet struct. One keeps track of the allocated
>> size of the NAL buffer, whereas the other keeps track of the amount of
>> NALs used by the current packet. That's why the check immediately above
>> this line makes sure the amount of NALs in the current packet fit in the
>> current size of the NAL buffer, and grows/reallocates it otherwise.
> 
> Thanks for the detail information about the two field. Below is my assumation 
> as I haven't such sample to test such case.
> Assuming nals_allocated is 1, nb_nals is 3

That can't happen. nb_nals is always set to zero at the start of
ff_h2645_packet_split(), and is increased by one every time a NAL is
finished parsing. It's guaranteed to always be less or equal than
nals_allocated by the above check.

, then the current code will reallocate
> new_size with 2, But the following code will use the nb_nals to access pkt->nals, 
> I'm not sure nals memory is allocated or not?
> nal = &pkt->nals[pkt->nb_nals];
> 
> 
>>
>>>              void *tmp = av_realloc_array(pkt->nals, new_size, sizeof(*pkt->nals));
>>>  
>>>              if (!tmp)
>>>
>>
>> _______________________________________________
>> 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".
>
lance.lmwang@gmail.com Sept. 10, 2019, 5:36 a.m.
On Tue, Sep 10, 2019 at 12:03:14AM -0300, James Almer wrote:
> On 9/9/2019 11:51 PM, Limin Wang wrote:
> > On Mon, Sep 09, 2019 at 10:53:53PM -0300, James Almer wrote:
> >> On 9/9/2019 10:18 PM, lance.lmwang@gmail.com wrote:
> >>> From: Limin Wang <lance.lmwang@gmail.com>
> >>>
> >>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >>> ---
> >>>  libavcodec/h2645_parse.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
> >>> index 307e8643e6..403acd2ee1 100644
> >>> --- a/libavcodec/h2645_parse.c
> >>> +++ b/libavcodec/h2645_parse.c
> >>> @@ -454,7 +454,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
> >>>          }
> >>>  
> >>>          if (pkt->nals_allocated < pkt->nb_nals + 1) {
> >>> -            int new_size = pkt->nals_allocated + 1;
> >>> +            int new_size = pkt->nb_nals + 1;
> >>
> >> No. nals_allocated and nb_nals are two purposely separate values in
> >> order to reuse the H2645Packet struct. One keeps track of the allocated
> >> size of the NAL buffer, whereas the other keeps track of the amount of
> >> NALs used by the current packet. That's why the check immediately above
> >> this line makes sure the amount of NALs in the current packet fit in the
> >> current size of the NAL buffer, and grows/reallocates it otherwise.
> > 
> > Thanks for the detail information about the two field. Below is my assumation 
> > as I haven't such sample to test such case.
> > Assuming nals_allocated is 1, nb_nals is 3
> 
> That can't happen. nb_nals is always set to zero at the start of
> ff_h2645_packet_split(), and is increased by one every time a NAL is
> finished parsing. It's guaranteed to always be less or equal than
> nals_allocated by the above check.
> 
> , then the current code will reallocate

Thanks, it's clear now. I have small change to get better code readiablity(which I think)


> > new_size with 2, But the following code will use the nb_nals to access pkt->nals, 
> > I'm not sure nals memory is allocated or not?
> > nal = &pkt->nals[pkt->nb_nals];
> > 
> > 
> >>
> >>>              void *tmp = av_realloc_array(pkt->nals, new_size, sizeof(*pkt->nals));
> >>>  
> >>>              if (!tmp)
> >>>
> >>
> >> _______________________________________________
> >> 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 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".

Patch hide | download patch | download mbox

diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
index 307e8643e6..403acd2ee1 100644
--- a/libavcodec/h2645_parse.c
+++ b/libavcodec/h2645_parse.c
@@ -454,7 +454,7 @@  int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
         }
 
         if (pkt->nals_allocated < pkt->nb_nals + 1) {
-            int new_size = pkt->nals_allocated + 1;
+            int new_size = pkt->nb_nals + 1;
             void *tmp = av_realloc_array(pkt->nals, new_size, sizeof(*pkt->nals));
 
             if (!tmp)