diff mbox

[FFmpeg-devel,V2] lavc/midivid: FIX CID 1456088

Message ID 1577028418-15051-1-git-send-email-zhongli_dev@126.com
State New
Headers show

Commit Message

Zhong Li Dec. 22, 2019, 3:26 p.m. UTC
Signed-off-by: Zhong Li <zhongli_dev@126.com>
---
 libavcodec/midivid.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

James Almer Dec. 22, 2019, 3:29 p.m. UTC | #1
On 12/22/2019 12:26 PM, Zhong Li wrote:
> Signed-off-by: Zhong Li <zhongli_dev@126.com>
> ---
>  libavcodec/midivid.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/midivid.c b/libavcodec/midivid.c
> index 38465c5..3dac3f1 100644
> --- a/libavcodec/midivid.c
> +++ b/libavcodec/midivid.c
> @@ -63,17 +63,20 @@ static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, AVFrame *frame)
>      if (intra_flag) {
>          nb_blocks = (avctx->width / 2) * (avctx->height / 2);
>      } else {
> -        int skip_linesize;
> +        int ret, skip_linesize;
>  
>          nb_blocks = bytestream2_get_le32(gb);
>          skip_linesize = avctx->width >> 1;
>          mask_start = gb->buffer_start + bytestream2_tell(gb);
>          mask_size = (avctx->width >> 5) * (avctx->height >> 2);
>  
> -        if (bytestream2_get_bytes_left(gb) < mask_size)
> +        ret = bytestream2_get_bytes_left(gb);
> +        if (ret < mask_size)

What is this fixing?

>              return AVERROR_INVALIDDATA;
>  
> -        init_get_bits8(&mask, mask_start, mask_size);
> +        ret = init_get_bits8(&mask, mask_start, mask_size);
> +        if (ret < 0)
> +            return AVERROR_INVALIDDATA;
>          bytestream2_skip(gb, mask_size);
>          skip = s->skip;
>  
>
Limin Wang Dec. 23, 2019, 12:53 a.m. UTC | #2
On Sun, Dec 22, 2019 at 03:26:58PM +0000, Zhong Li wrote:
> Signed-off-by: Zhong Li <zhongli_dev@126.com>
> ---
>  libavcodec/midivid.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/midivid.c b/libavcodec/midivid.c
> index 38465c5..3dac3f1 100644
> --- a/libavcodec/midivid.c
> +++ b/libavcodec/midivid.c
> @@ -63,17 +63,20 @@ static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, AVFrame *frame)
>      if (intra_flag) {
>          nb_blocks = (avctx->width / 2) * (avctx->height / 2);
>      } else {
> -        int skip_linesize;
> +        int ret, skip_linesize;
>  
>          nb_blocks = bytestream2_get_le32(gb);
>          skip_linesize = avctx->width >> 1;
>          mask_start = gb->buffer_start + bytestream2_tell(gb);
>          mask_size = (avctx->width >> 5) * (avctx->height >> 2);
>  
> -        if (bytestream2_get_bytes_left(gb) < mask_size)
> +        ret = bytestream2_get_bytes_left(gb);
> +        if (ret < mask_size)
>              return AVERROR_INVALIDDATA;

don't need change it I think.

>  
> -        init_get_bits8(&mask, mask_start, mask_size);
> +        ret = init_get_bits8(&mask, mask_start, mask_size);
> +        if (ret < 0)
> +            return AVERROR_INVALIDDATA;

return ret. That's why use ret.

>          bytestream2_skip(gb, mask_size);
>          skip = s->skip;
>  
> -- 
> 1.8.3.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".
Andreas Rheinhardt Dec. 23, 2019, 1 a.m. UTC | #3
On Mon, Dec 23, 2019 at 1:53 AM Limin Wang <lance.lmwang@gmail.com> wrote:

> On Sun, Dec 22, 2019 at 03:26:58PM +0000, Zhong Li wrote:
> > Signed-off-by: Zhong Li <zhongli_dev@126.com>
> > ---
> >  libavcodec/midivid.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavcodec/midivid.c b/libavcodec/midivid.c
> > index 38465c5..3dac3f1 100644
> > --- a/libavcodec/midivid.c
> > +++ b/libavcodec/midivid.c
> > @@ -63,17 +63,20 @@ static int decode_mvdv(MidiVidContext *s,
> AVCodecContext *avctx, AVFrame *frame)
> >      if (intra_flag) {
> >          nb_blocks = (avctx->width / 2) * (avctx->height / 2);
> >      } else {
> > -        int skip_linesize;
> > +        int ret, skip_linesize;
> >
> >          nb_blocks = bytestream2_get_le32(gb);
> >          skip_linesize = avctx->width >> 1;
> >          mask_start = gb->buffer_start + bytestream2_tell(gb);
> >          mask_size = (avctx->width >> 5) * (avctx->height >> 2);
> >
> > -        if (bytestream2_get_bytes_left(gb) < mask_size)
> > +        ret = bytestream2_get_bytes_left(gb);
> > +        if (ret < mask_size)
> >              return AVERROR_INVALIDDATA;
>
> don't need change it I think.
>
> >
> > -        init_get_bits8(&mask, mask_start, mask_size);
> > +        ret = init_get_bits8(&mask, mask_start, mask_size);
> > +        if (ret < 0)
> > +            return AVERROR_INVALIDDATA;
>
> return ret. That's why use ret.
>
>
But you are not returning ret.

- Andreas
Limin Wang Dec. 23, 2019, 4:55 a.m. UTC | #4
On Mon, Dec 23, 2019 at 02:00:10AM +0100, Andreas Rheinhardt wrote:
> On Mon, Dec 23, 2019 at 1:53 AM Limin Wang <lance.lmwang@gmail.com> wrote:
> 
> > On Sun, Dec 22, 2019 at 03:26:58PM +0000, Zhong Li wrote:
> > > Signed-off-by: Zhong Li <zhongli_dev@126.com>
> > > ---
> > >  libavcodec/midivid.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/libavcodec/midivid.c b/libavcodec/midivid.c
> > > index 38465c5..3dac3f1 100644
> > > --- a/libavcodec/midivid.c
> > > +++ b/libavcodec/midivid.c
> > > @@ -63,17 +63,20 @@ static int decode_mvdv(MidiVidContext *s,
> > AVCodecContext *avctx, AVFrame *frame)
> > >      if (intra_flag) {
> > >          nb_blocks = (avctx->width / 2) * (avctx->height / 2);
> > >      } else {
> > > -        int skip_linesize;
> > > +        int ret, skip_linesize;
> > >
> > >          nb_blocks = bytestream2_get_le32(gb);
> > >          skip_linesize = avctx->width >> 1;
> > >          mask_start = gb->buffer_start + bytestream2_tell(gb);
> > >          mask_size = (avctx->width >> 5) * (avctx->height >> 2);
> > >
> > > -        if (bytestream2_get_bytes_left(gb) < mask_size)
> > > +        ret = bytestream2_get_bytes_left(gb);
> > > +        if (ret < mask_size)
> > >              return AVERROR_INVALIDDATA;
> >
> > don't need change it I think.
> >
> > >
> > > -        init_get_bits8(&mask, mask_start, mask_size);
> > > +        ret = init_get_bits8(&mask, mask_start, mask_size);
> > > +        if (ret < 0)
> > > +            return AVERROR_INVALIDDATA;
> >
> > return ret. That's why use ret.
> >
> >
> But you are not returning ret.

But It's not my code, I'm a code reviewer.

> 
> - 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".
Andreas Rheinhardt Dec. 23, 2019, 5:13 a.m. UTC | #5
On Mon, Dec 23, 2019 at 6:03 AM Limin Wang <lance.lmwang@gmail.com> wrote:

> On Mon, Dec 23, 2019 at 02:00:10AM +0100, Andreas Rheinhardt wrote:
> > On Mon, Dec 23, 2019 at 1:53 AM Limin Wang <lance.lmwang@gmail.com>
> wrote:
> >
> > > On Sun, Dec 22, 2019 at 03:26:58PM +0000, Zhong Li wrote:
> > > > Signed-off-by: Zhong Li <zhongli_dev@126.com>
> > > > ---
> > > >  libavcodec/midivid.c | 9 ++++++---
> > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/libavcodec/midivid.c b/libavcodec/midivid.c
> > > > index 38465c5..3dac3f1 100644
> > > > --- a/libavcodec/midivid.c
> > > > +++ b/libavcodec/midivid.c
> > > > @@ -63,17 +63,20 @@ static int decode_mvdv(MidiVidContext *s,
> > > AVCodecContext *avctx, AVFrame *frame)
> > > >      if (intra_flag) {
> > > >          nb_blocks = (avctx->width / 2) * (avctx->height / 2);
> > > >      } else {
> > > > -        int skip_linesize;
> > > > +        int ret, skip_linesize;
> > > >
> > > >          nb_blocks = bytestream2_get_le32(gb);
> > > >          skip_linesize = avctx->width >> 1;
> > > >          mask_start = gb->buffer_start + bytestream2_tell(gb);
> > > >          mask_size = (avctx->width >> 5) * (avctx->height >> 2);
> > > >
> > > > -        if (bytestream2_get_bytes_left(gb) < mask_size)
> > > > +        ret = bytestream2_get_bytes_left(gb);
> > > > +        if (ret < mask_size)
> > > >              return AVERROR_INVALIDDATA;
> > >
> > > don't need change it I think.
> > >
> > > >
> > > > -        init_get_bits8(&mask, mask_start, mask_size);
> > > > +        ret = init_get_bits8(&mask, mask_start, mask_size);
> > > > +        if (ret < 0)
> > > > +            return AVERROR_INVALIDDATA;
> > >
> > > return ret. That's why use ret.
> > >
> > >
> > But you are not returning ret.
>
> But It's not my code, I'm a code reviewer.
>
>
You are right. Forget my previous mail, it's just nonsense then.

- Andreas
Nicolas George Dec. 23, 2019, 9 a.m. UTC | #6
Limin Wang (12019-12-23):
> But It's not my code, I'm a code reviewer.

Then review that ret needs to be returned ;-) It needs more change, not
less.

And the commit message needs to be explicit. CID may not be available
everywhere or everywhen.

Regards,
Zhong Li Dec. 23, 2019, 12:31 p.m. UTC | #7
James Almer <jamrial@gmail.com> 于2019年12月22日周日 下午11:29写道:
>
> On 12/22/2019 12:26 PM, Zhong Li wrote:
> > Signed-off-by: Zhong Li <zhongli_dev@126.com>
> > ---
> >  libavcodec/midivid.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavcodec/midivid.c b/libavcodec/midivid.c
> > index 38465c5..3dac3f1 100644
> > --- a/libavcodec/midivid.c
> > +++ b/libavcodec/midivid.c
> > @@ -63,17 +63,20 @@ static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, AVFrame *frame)
> >      if (intra_flag) {
> >          nb_blocks = (avctx->width / 2) * (avctx->height / 2);
> >      } else {
> > -        int skip_linesize;
> > +        int ret, skip_linesize;
> >
> >          nb_blocks = bytestream2_get_le32(gb);
> >          skip_linesize = avctx->width >> 1;
> >          mask_start = gb->buffer_start + bytestream2_tell(gb);
> >          mask_size = (avctx->width >> 5) * (avctx->height >> 2);
> >
> > -        if (bytestream2_get_bytes_left(gb) < mask_size)
> > +        ret = bytestream2_get_bytes_left(gb);
> > +        if (ret < mask_size)
>
> What is this fixing?

Not fix. Just want to use same coding stytle as following.
Zhong Li Dec. 23, 2019, 12:38 p.m. UTC | #8
Nicolas George <george@nsup.org> 于2019年12月23日周一 下午5:00写道:
>
> Limin Wang (12019-12-23):
> > But It's not my code, I'm a code reviewer.
>
> Then review that ret needs to be returned ;-) It needs more change, not
> less.

Could specify what "more change" is needed?
Nicolas George Dec. 23, 2019, 12:39 p.m. UTC | #9
Zhong Li (12019-12-23):
> > Then review that ret needs to be returned ;-) It needs more change, not
                     ^^^^^^^^^^^^^^^^^^^^^^^^
> > less.
> 
> Could specify what "more change" is needed?

See above, I underlined it.

Regards,
Paul B Mahol Dec. 23, 2019, 12:58 p.m. UTC | #10
On 12/22/19, Zhong Li <zhongli_dev@126.com> wrote:
> Signed-off-by: Zhong Li <zhongli_dev@126.com>
> ---
>  libavcodec/midivid.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/libavcodec/midivid.c b/libavcodec/midivid.c
> index 38465c5..3dac3f1 100644
> --- a/libavcodec/midivid.c
> +++ b/libavcodec/midivid.c
> @@ -63,17 +63,20 @@ static int decode_mvdv(MidiVidContext *s, AVCodecContext
> *avctx, AVFrame *frame)
>      if (intra_flag) {
>          nb_blocks = (avctx->width / 2) * (avctx->height / 2);
>      } else {
> -        int skip_linesize;
> +        int ret, skip_linesize;
>
>          nb_blocks = bytestream2_get_le32(gb);
>          skip_linesize = avctx->width >> 1;
>          mask_start = gb->buffer_start + bytestream2_tell(gb);
>          mask_size = (avctx->width >> 5) * (avctx->height >> 2);
>
> -        if (bytestream2_get_bytes_left(gb) < mask_size)
> +        ret = bytestream2_get_bytes_left(gb);
> +        if (ret < mask_size)
>              return AVERROR_INVALIDDATA;

Pointless

>
> -        init_get_bits8(&mask, mask_start, mask_size);
> +        ret = init_get_bits8(&mask, mask_start, mask_size);
> +        if (ret < 0)
> +            return AVERROR_INVALIDDATA;

ret should be returned here

>          bytestream2_skip(gb, mask_size);
>          skip = s->skip;
>
> --
> 1.8.3.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".
James Almer Dec. 23, 2019, 1:09 p.m. UTC | #11
On 12/23/2019 9:31 AM, Zhong Li wrote:
> James Almer <jamrial@gmail.com> 于2019年12月22日周日 下午11:29写道:
>>
>> On 12/22/2019 12:26 PM, Zhong Li wrote:
>>> Signed-off-by: Zhong Li <zhongli_dev@126.com>
>>> ---
>>>  libavcodec/midivid.c | 9 ++++++---
>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/libavcodec/midivid.c b/libavcodec/midivid.c
>>> index 38465c5..3dac3f1 100644
>>> --- a/libavcodec/midivid.c
>>> +++ b/libavcodec/midivid.c
>>> @@ -63,17 +63,20 @@ static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, AVFrame *frame)
>>>      if (intra_flag) {
>>>          nb_blocks = (avctx->width / 2) * (avctx->height / 2);
>>>      } else {
>>> -        int skip_linesize;
>>> +        int ret, skip_linesize;
>>>
>>>          nb_blocks = bytestream2_get_le32(gb);
>>>          skip_linesize = avctx->width >> 1;
>>>          mask_start = gb->buffer_start + bytestream2_tell(gb);
>>>          mask_size = (avctx->width >> 5) * (avctx->height >> 2);
>>>
>>> -        if (bytestream2_get_bytes_left(gb) < mask_size)
>>> +        ret = bytestream2_get_bytes_left(gb);
>>> +        if (ret < mask_size)
>>
>> What is this fixing?
> 
> Not fix. Just want to use same coding stytle as following.

Unless you need it to return a value, then no point in making this change.

Add the init_get_bits8() check only, and make it return ret as others
suggested. Also, the commit subject should be something like
"lavc/midivid: check return value of init_get_bits8()", and the CID
1456088 comment added further down in the commit message.
Zhong Li Dec. 23, 2019, 1:23 p.m. UTC | #12
James Almer <jamrial@gmail.com> 于2019年12月23日周一 下午9:09写道:
>
> On 12/23/2019 9:31 AM, Zhong Li wrote:
> > James Almer <jamrial@gmail.com> 于2019年12月22日周日 下午11:29写道:
> >>
> >> On 12/22/2019 12:26 PM, Zhong Li wrote:
> >>> Signed-off-by: Zhong Li <zhongli_dev@126.com>
> >>> ---
> >>>  libavcodec/midivid.c | 9 ++++++---
> >>>  1 file changed, 6 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/libavcodec/midivid.c b/libavcodec/midivid.c
> >>> index 38465c5..3dac3f1 100644
> >>> --- a/libavcodec/midivid.c
> >>> +++ b/libavcodec/midivid.c
> >>> @@ -63,17 +63,20 @@ static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, AVFrame *frame)
> >>>      if (intra_flag) {
> >>>          nb_blocks = (avctx->width / 2) * (avctx->height / 2);
> >>>      } else {
> >>> -        int skip_linesize;
> >>> +        int ret, skip_linesize;
> >>>
> >>>          nb_blocks = bytestream2_get_le32(gb);
> >>>          skip_linesize = avctx->width >> 1;
> >>>          mask_start = gb->buffer_start + bytestream2_tell(gb);
> >>>          mask_size = (avctx->width >> 5) * (avctx->height >> 2);
> >>>
> >>> -        if (bytestream2_get_bytes_left(gb) < mask_size)
> >>> +        ret = bytestream2_get_bytes_left(gb);
> >>> +        if (ret < mask_size)
> >>
> >> What is this fixing?
> >
> > Not fix. Just want to use same coding stytle as following.
>
> Unless you need it to return a value, then no point in making this change.
>
> Add the init_get_bits8() check only, and make it return ret as others
> suggested. Also, the commit subject should be something like
> "lavc/midivid: check return value of init_get_bits8()", and the CID
> 1456088 comment added further down in the commit message.

Yeah, thanks for comments. Will update.
diff mbox

Patch

diff --git a/libavcodec/midivid.c b/libavcodec/midivid.c
index 38465c5..3dac3f1 100644
--- a/libavcodec/midivid.c
+++ b/libavcodec/midivid.c
@@ -63,17 +63,20 @@  static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx, AVFrame *frame)
     if (intra_flag) {
         nb_blocks = (avctx->width / 2) * (avctx->height / 2);
     } else {
-        int skip_linesize;
+        int ret, skip_linesize;
 
         nb_blocks = bytestream2_get_le32(gb);
         skip_linesize = avctx->width >> 1;
         mask_start = gb->buffer_start + bytestream2_tell(gb);
         mask_size = (avctx->width >> 5) * (avctx->height >> 2);
 
-        if (bytestream2_get_bytes_left(gb) < mask_size)
+        ret = bytestream2_get_bytes_left(gb);
+        if (ret < mask_size)
             return AVERROR_INVALIDDATA;
 
-        init_get_bits8(&mask, mask_start, mask_size);
+        ret = init_get_bits8(&mask, mask_start, mask_size);
+        if (ret < 0)
+            return AVERROR_INVALIDDATA;
         bytestream2_skip(gb, mask_size);
         skip = s->skip;