diff mbox series

[FFmpeg-devel] libavfilter: avoid UB nullptr-with-offset.

Message ID 20210216155308.3327069-1-jleconte@google.com
State New
Headers show
Series [FFmpeg-devel] libavfilter: avoid UB nullptr-with-offset.
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate fail Make fate failed
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate warning Make fate failed

Commit Message

Jeremy Leconte Feb. 16, 2021, 3:53 p.m. UTC
---
 libavfilter/vf_scale.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andreas Rheinhardt Feb. 16, 2021, 3:58 p.m. UTC | #1
Jeremy Leconte:
> ---
>  libavfilter/vf_scale.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> index 58eee96744..98cef5eb4b 100644
> --- a/libavfilter/vf_scale.c
> +++ b/libavfilter/vf_scale.c
> @@ -631,7 +631,7 @@ static int scale_slice(AVFilterLink *link, AVFrame *out_buf, AVFrame *cur_pic, s
>      int in_stride[4],out_stride[4];
>      int i;
>  
> -    for (i=0; i<4; i++) {
> +    for (i=0; i<4 && cur_pic->data[i] != NULL; i++) {
>          int vsub= ((i+1)&2) ? scale->vsub : 0;
>           in_stride[i] = cur_pic->linesize[i] * mul;
>          out_stride[i] = out_buf->linesize[i] * mul;
> 
Is this just a case of NULL + 0 or is the offset != 0?

- Andreas
Jeremy Leconte Feb. 16, 2021, 4:13 p.m. UTC | #2
You're right, it's just a case of NULL + 0.
The problem is that it gets caught by clang -fsanitize=undefined.

On Tue, Feb 16, 2021 at 4:59 PM Andreas Rheinhardt <
andreas.rheinhardt@gmail.com> wrote:

> Jeremy Leconte:
> > ---
> >  libavfilter/vf_scale.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> > index 58eee96744..98cef5eb4b 100644
> > --- a/libavfilter/vf_scale.c
> > +++ b/libavfilter/vf_scale.c
> > @@ -631,7 +631,7 @@ static int scale_slice(AVFilterLink *link, AVFrame
> *out_buf, AVFrame *cur_pic, s
> >      int in_stride[4],out_stride[4];
> >      int i;
> >
> > -    for (i=0; i<4; i++) {
> > +    for (i=0; i<4 && cur_pic->data[i] != NULL; i++) {
> >          int vsub= ((i+1)&2) ? scale->vsub : 0;
> >           in_stride[i] = cur_pic->linesize[i] * mul;
> >          out_stride[i] = out_buf->linesize[i] * mul;
> >
> Is this just a case of NULL + 0 or is the offset != 0?
>
> - 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".
Jeremy Leconte Feb. 16, 2021, 4:24 p.m. UTC | #3
Andreas Rheinhardt:
> Is this just a case of NULL + 0 or is the offset != 0?
>
> - Andreas
>
You're right, it's just a case of NULL + 0.
My issue is that it gets caught by clang -fsanitize=undefined.

Jeremy
Michael Niedermayer Feb. 17, 2021, 10:45 p.m. UTC | #4
On Tue, Feb 16, 2021 at 03:53:08PM +0000, Jeremy Leconte wrote:
> ---
>  libavfilter/vf_scale.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> index 58eee96744..98cef5eb4b 100644
> --- a/libavfilter/vf_scale.c
> +++ b/libavfilter/vf_scale.c
> @@ -631,7 +631,7 @@ static int scale_slice(AVFilterLink *link, AVFrame *out_buf, AVFrame *cur_pic, s
>      int in_stride[4],out_stride[4];
>      int i;
>  
> -    for (i=0; i<4; i++) {
> +    for (i=0; i<4 && cur_pic->data[i] != NULL; i++) {
>          int vsub= ((i+1)&2) ? scale->vsub : 0;
>           in_stride[i] = cur_pic->linesize[i] * mul;
>          out_stride[i] = out_buf->linesize[i] * mul;

This leaves parts of the array uninitialized 
Ill fix this (unless someone else does before me)

thx

[...]
Andreas Rheinhardt Feb. 17, 2021, 10:50 p.m. UTC | #5
Michael Niedermayer:
> On Tue, Feb 16, 2021 at 03:53:08PM +0000, Jeremy Leconte wrote:
>> ---
>>  libavfilter/vf_scale.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
>> index 58eee96744..98cef5eb4b 100644
>> --- a/libavfilter/vf_scale.c
>> +++ b/libavfilter/vf_scale.c
>> @@ -631,7 +631,7 @@ static int scale_slice(AVFilterLink *link, AVFrame *out_buf, AVFrame *cur_pic, s
>>      int in_stride[4],out_stride[4];
>>      int i;
>>  
>> -    for (i=0; i<4; i++) {
>> +    for (i=0; i<4 && cur_pic->data[i] != NULL; i++) {
>>          int vsub= ((i+1)&2) ? scale->vsub : 0;
>>           in_stride[i] = cur_pic->linesize[i] * mul;
>>          out_stride[i] = out_buf->linesize[i] * mul;
> 
> This leaves parts of the array uninitialized 
> Ill fix this (unless someone else does before me)
> 
I noticed that GCC is smart enough to optimize a
ptr = addend ? ptr + addend : addend
to a simple ptr += addend (Clang isn't, but future version might do so).
Maybe we should put the above in a macro and use it everywhere where
such problems exist?
(It would of course not fix the case of NULL + addend with addend != 0,
but such cases are genuine logic bugs that should not be hidden, but fixed.)

- Andreas
diff mbox series

Patch

diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
index 58eee96744..98cef5eb4b 100644
--- a/libavfilter/vf_scale.c
+++ b/libavfilter/vf_scale.c
@@ -631,7 +631,7 @@  static int scale_slice(AVFilterLink *link, AVFrame *out_buf, AVFrame *cur_pic, s
     int in_stride[4],out_stride[4];
     int i;
 
-    for (i=0; i<4; i++) {
+    for (i=0; i<4 && cur_pic->data[i] != NULL; i++) {
         int vsub= ((i+1)&2) ? scale->vsub : 0;
          in_stride[i] = cur_pic->linesize[i] * mul;
         out_stride[i] = out_buf->linesize[i] * mul;