diff mbox series

[FFmpeg-devel,v2,2/3] avfilter/vf_mix: Check sscanf() return value

Message ID 20200328001733.695-2-lance.lmwang@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,v2,1/3] avfilter/af_acrossover: Check sscanf() return value
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Limin Wang March 28, 2020, 12:17 a.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavfilter/vf_mix.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Nicolas George March 28, 2020, 7:10 p.m. UTC | #1
lance.lmwang@gmail.com (12020-03-28):
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavfilter/vf_mix.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Why? What do you expect to change with this patch, and how is it
supposed to be better?

Regards,
Limin Wang March 28, 2020, 11:18 p.m. UTC | #2
On Sat, Mar 28, 2020 at 08:10:51PM +0100, Nicolas George wrote:
> lance.lmwang@gmail.com (12020-03-28):
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavfilter/vf_mix.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Why? What do you expect to change with this patch, and how is it
> supposed to be better?
If av_sscanf failed,  s->weights[i] is invalid data and can't be used
further. As the old code allow to use the last weights, so I use 
continue to avoid invalid access only. Do you think it's better to
print out one warning message?

> 
> Regards,
> 
> -- 
>   Nicolas George
Nicolas George March 28, 2020, 11:31 p.m. UTC | #3
Limin Wang (12020-03-29):
> If av_sscanf failed,  s->weights[i] is invalid data and can't be used
> further.

I think you are mistaken: if av_sscanf() fails, s->weights[i] stays
unchanged: it was 0, it's still 0.

> As the old code allow to use the last weights, so I use 
> continue to avoid invalid access only.

It's not invalid, it's 0.

> Do you think it's better to
> print out one warning message?

With this correction, I think this change is not necessary at all.

Regards,
Limin Wang March 29, 2020, 12:06 a.m. UTC | #4
On Sun, Mar 29, 2020 at 12:31:40AM +0100, Nicolas George wrote:
> Limin Wang (12020-03-29):
> > If av_sscanf failed,  s->weights[i] is invalid data and can't be used
> > further.
> 
> I think you are mistaken: if av_sscanf() fails, s->weights[i] stays
> unchanged: it was 0, it's still 0.
> 
> > As the old code allow to use the last weights, so I use 
> > continue to avoid invalid access only.
> 
> It's not invalid, it's 0.
> 
> > Do you think it's better to
> > print out one warning message?
> 
> With this correction, I think this change is not necessary at all.

Yes, the result is same with my change. Anyway if the user option is typo, 
the result may be unexpected for user. 

> 
> Regards,
> 
> -- 
>   Nicolas George



> _______________________________________________
> 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".
Nicolas George March 29, 2020, 12:32 a.m. UTC | #5
Limin Wang (12020-03-29):
> Anyway if the user option is typo, 
> the result may be unexpected for user. 

But your change does not address that. For that, you would print a
message and return an error. Which is probably the right thing to do,
actually.

Regards,
Limin Wang March 29, 2020, 1:09 a.m. UTC | #6
On Sun, Mar 29, 2020 at 01:32:59AM +0100, Nicolas George wrote:
> Limin Wang (12020-03-29):
> > Anyway if the user option is typo, 
> > the result may be unexpected for user. 
> 
> But your change does not address that. For that, you would print a
> message and return an error. Which is probably the right thing to do,
> actually.

OK, update the patch to print a message and return error.


> 
> Regards,
> 
> -- 
>   Nicolas George



> _______________________________________________
> 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".
Nicolas George March 29, 2020, 9:18 a.m. UTC | #7
Limin Wang (12020-03-29):
> OK, update the patch to print a message and return error.

Thanks. I have no objection to that new version, but I do not maintain
that code.

Regards,
Limin Wang April 8, 2020, 1:59 p.m. UTC | #8
On Sun, Mar 29, 2020 at 11:18:11AM +0200, Nicolas George wrote:
> Limin Wang (12020-03-29):
> > OK, update the patch to print a message and return error.
> 
> Thanks. I have no objection to that new version, but I do not maintain
> that code.

ping for the patchset for merge.

> 
> Regards,
> 
> -- 
>   Nicolas George
> _______________________________________________
> 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".
diff mbox series

Patch

diff --git a/libavfilter/vf_mix.c b/libavfilter/vf_mix.c
index 9e1ae79..e47f7e2 100644
--- a/libavfilter/vf_mix.c
+++ b/libavfilter/vf_mix.c
@@ -108,7 +108,8 @@  static av_cold int init(AVFilterContext *ctx)
             break;
 
         p = NULL;
-        av_sscanf(arg, "%f", &s->weights[i]);
+        if (av_sscanf(arg, "%f", &s->weights[i]) != 1)
+            continue;
         s->wfactor += s->weights[i];
         last = i;
     }