| Message ID | CAGn-TggLSO73Eie9PDSSChvvotF89AV+vLKe14Jv_f7F=8tCFQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sun, May 12, 2019 at 05:40:00AM -0700, Adam Richter wrote: > This is a possible fix for a string overflow in some sscanf calls in > libswcale/tests/swscale.c, in the function fileTest(), found by > cppcheck. Please see the attachment for more discussion of this. > > Thanks in advance for considering this patch. > > Adam > swscale.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > 337bfa52e3917c2d896ca5c7ba1b669d5970cdab 0002-libswcale-Fix-possible-string-overflow-in-test.patch > From 8b5f994bcd2576588149f228695823b5cf8d3dc8 Mon Sep 17 00:00:00 2001 > From: Adam Richter <adamrichter4@gmail.com> > Date: Sun, 12 May 2019 05:03:25 -0700 > Subject: [PATCH] libswcale: Fix possible string overflow in test. > > In libswcale/tests/swcale.c, the function fileTest() calls sscanf in > an argument of "%12s" on character srcStr[] and dstStr[], which are > only 12 bytes. So, if the input string is 12 characters, a > terminating null byte can be written past the end of these arrays. > > This bug was found by cppcheck. > > I am not an ffmpeg or libswcale developer, and I believe that this is > the first patch I am submitting to ffmpeg, so please let me know if > I am doing anything wrong in the patch submission process. > > For the same reason, please examine this patch skeptically, especially > considering that I have not tested this patch other than to see that > it compiled without complaint and that "make fate" completed with a > zero exit code. I do not know if this program actually > expects these input strings to be a maximum of 11 or 12 characters long. > In this patch, I assume that they could be 12 characters long, so I have > extended the array sizes, but perhaps a more correct fix might > be to change the "%12s" instances to "%11s" instead. > > Thanks in advance for considering this patch. I actually think 13 is not long enough for the longest name. Ill fix it, thanks for finding this [...]
Hi, Michael. Thanks for reviewing that possible string overflow found by cppcheck and proposing to try to make a better fix. I'll assume no further action on my part for this fix is necessary unless anyone tells me otherwise. Adam Adam On Mon, May 13, 2019 at 4:39 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Sun, May 12, 2019 at 05:40:00AM -0700, Adam Richter wrote: > > This is a possible fix for a string overflow in some sscanf calls in > > libswcale/tests/swscale.c, in the function fileTest(), found by > > cppcheck. Please see the attachment for more discussion of this. > > > > Thanks in advance for considering this patch. > > > > Adam > > > swscale.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > 337bfa52e3917c2d896ca5c7ba1b669d5970cdab 0002-libswcale-Fix-possible-string-overflow-in-test.patch > > From 8b5f994bcd2576588149f228695823b5cf8d3dc8 Mon Sep 17 00:00:00 2001 > > From: Adam Richter <adamrichter4@gmail.com> > > Date: Sun, 12 May 2019 05:03:25 -0700 > > Subject: [PATCH] libswcale: Fix possible string overflow in test. > > > > In libswcale/tests/swcale.c, the function fileTest() calls sscanf in > > an argument of "%12s" on character srcStr[] and dstStr[], which are > > only 12 bytes. So, if the input string is 12 characters, a > > terminating null byte can be written past the end of these arrays. > > > > This bug was found by cppcheck. > > > > I am not an ffmpeg or libswcale developer, and I believe that this is > > the first patch I am submitting to ffmpeg, so please let me know if > > I am doing anything wrong in the patch submission process. > > > > For the same reason, please examine this patch skeptically, especially > > considering that I have not tested this patch other than to see that > > it compiled without complaint and that "make fate" completed with a > > zero exit code. I do not know if this program actually > > expects these input strings to be a maximum of 11 or 12 characters long. > > In this patch, I assume that they could be 12 characters long, so I have > > extended the array sizes, but perhaps a more correct fix might > > be to change the "%12s" instances to "%11s" instead. > > > > Thanks in advance for considering this patch. > > I actually think 13 is not long enough for the longest name. > Ill fix it, thanks for finding this > > > [...] > > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Rewriting code that is poorly written but fully understood is good. > Rewriting code that one doesnt understand is a sign that one is less smart > then the original author, trying to rewrite it will not make it better. > _______________________________________________ > 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".
Am Mo., 13. Mai 2019 um 20:33 Uhr schrieb Adam Richter <adamrichter4@gmail.com>: > Thanks for reviewing that possible string overflow found by cppcheck > and proposing to try to make a better fix. I'll assume no further > action on my part for this fix is necessary unless anyone tells me > otherwise. See: http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=9d269301 Please remember not to top-post here, Carl Eugen
From 8b5f994bcd2576588149f228695823b5cf8d3dc8 Mon Sep 17 00:00:00 2001 From: Adam Richter <adamrichter4@gmail.com> Date: Sun, 12 May 2019 05:03:25 -0700 Subject: [PATCH] libswcale: Fix possible string overflow in test. In libswcale/tests/swcale.c, the function fileTest() calls sscanf in an argument of "%12s" on character srcStr[] and dstStr[], which are only 12 bytes. So, if the input string is 12 characters, a terminating null byte can be written past the end of these arrays. This bug was found by cppcheck. I am not an ffmpeg or libswcale developer, and I believe that this is the first patch I am submitting to ffmpeg, so please let me know if I am doing anything wrong in the patch submission process. For the same reason, please examine this patch skeptically, especially considering that I have not tested this patch other than to see that it compiled without complaint and that "make fate" completed with a zero exit code. I do not know if this program actually expects these input strings to be a maximum of 11 or 12 characters long. In this patch, I assume that they could be 12 characters long, so I have extended the array sizes, but perhaps a more correct fix might be to change the "%12s" instances to "%11s" instead. Thanks in advance for considering this patch. Signed-off-by: Adam Richter <adamrichter4@gmail.com> --- libswscale/tests/swscale.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libswscale/tests/swscale.c b/libswscale/tests/swscale.c index e72c4c3306..cb731f6211 100644 --- a/libswscale/tests/swscale.c +++ b/libswscale/tests/swscale.c @@ -312,10 +312,10 @@ static int fileTest(const uint8_t * const ref[4], int refStride[4], while (fgets(buf, sizeof(buf), fp)) { struct Results r; enum AVPixelFormat srcFormat; - char srcStr[12]; + char srcStr[13]; int srcW = 0, srcH = 0; enum AVPixelFormat dstFormat; - char dstStr[12]; + char dstStr[13]; int dstW = 0, dstH = 0; int flags; int ret; -- 2.21.0