| Message ID | 20190518073906.29792-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | fbdb3aa179939616bc7fb26e3fe386b85b58e467 |
| Headers | show |
On 2019-05-18 09:39, Michael Niedermayer wrote: > Fixes: "null pointer dereference" > Fixes: 14551/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_V210_fuzzer-5088609952071680 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/v210dec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/v210dec.c b/libavcodec/v210dec.c > index bc1e1d34ff..5a33d8c089 100644 > --- a/libavcodec/v210dec.c > +++ b/libavcodec/v210dec.c > @@ -104,7 +104,7 @@ static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, > && avpkt->size - 64 >= stride * avctx->height) > psrc += 64; > > - aligned_input = !((uintptr_t)psrc & 0xf) && !(stride & 0xf); > + aligned_input = !((uintptr_t)psrc & 0x1f) && !(stride & 0x1f); > if (aligned_input != s->aligned_input) { > s->aligned_input = aligned_input; > ff_v210dec_init(s); > Ah yes, that'll be needed after the recent addition of avx2. LGTM and sorry. I object to the commit message though because it isn't a "null pointer dereference" but if that is the error as reported by the tool then keep it as is.
On Sat, May 18, 2019 at 12:02:55PM +0200, James Darnley wrote: > On 2019-05-18 09:39, Michael Niedermayer wrote: > > Fixes: "null pointer dereference" > > Fixes: 14551/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_V210_fuzzer-5088609952071680 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/v210dec.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavcodec/v210dec.c b/libavcodec/v210dec.c > > index bc1e1d34ff..5a33d8c089 100644 > > --- a/libavcodec/v210dec.c > > +++ b/libavcodec/v210dec.c > > @@ -104,7 +104,7 @@ static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, > > && avpkt->size - 64 >= stride * avctx->height) > > psrc += 64; > > > > - aligned_input = !((uintptr_t)psrc & 0xf) && !(stride & 0xf); > > + aligned_input = !((uintptr_t)psrc & 0x1f) && !(stride & 0x1f); > > if (aligned_input != s->aligned_input) { > > s->aligned_input = aligned_input; > > ff_v210dec_init(s); > > > > Ah yes, that'll be needed after the recent addition of avx2. LGTM and > sorry. > > I object to the commit message though because it isn't a "null pointer > dereference" but if that is the error as reported by the tool then keep > it as is. yes, the tool(s) say things like "Null-dereference READ", "SEGV on unknown address 0x000000000000" [...]
On 18.05.2019, at 12:15, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Sat, May 18, 2019 at 12:02:55PM +0200, James Darnley wrote: >> On 2019-05-18 09:39, Michael Niedermayer wrote: >>> Fixes: "null pointer dereference" >>> Fixes: 14551/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_V210_fuzzer-5088609952071680 >>> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavcodec/v210dec.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavcodec/v210dec.c b/libavcodec/v210dec.c >>> index bc1e1d34ff..5a33d8c089 100644 >>> --- a/libavcodec/v210dec.c >>> +++ b/libavcodec/v210dec.c >>> @@ -104,7 +104,7 @@ static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, >>> && avpkt->size - 64 >= stride * avctx->height) >>> psrc += 64; >>> >>> - aligned_input = !((uintptr_t)psrc & 0xf) && !(stride & 0xf); >>> + aligned_input = !((uintptr_t)psrc & 0x1f) && !(stride & 0x1f); >>> if (aligned_input != s->aligned_input) { >>> s->aligned_input = aligned_input; >>> ff_v210dec_init(s); >>> >> >> Ah yes, that'll be needed after the recent addition of avx2. LGTM and >> sorry. >> > >> I object to the commit message though because it isn't a "null pointer >> dereference" but if that is the error as reported by the tool then keep >> it as is. > > yes, the tool(s) say things like "Null-dereference READ", "SEGV on unknown address 0x000000000000" Is that a tool bug or are we missing something that goes on here besides the alignment? If it's a tool bug can we try to get it fixed? This has a big risk of wasting time if they print straight-up wrong information...
On 2019-05-18 12:15, Michael Niedermayer wrote: > On Sat, May 18, 2019 at 12:02:55PM +0200, James Darnley wrote: >> I object to the commit message though because it isn't a "null pointer >> dereference" but if that is the error as reported by the tool then keep >> it as is. > > yes, the tool(s) say things like "Null-dereference READ", "SEGV on unknown address 0x000000000000" > Hm. It is almost certainly an aligned move on an unaligned address. I don't care that much about the rest of the commit message; the subject is correct which is good enough.
On Sat, May 18, 2019 at 01:05:01PM +0200, Reimar Döffinger wrote: > On 18.05.2019, at 12:15, Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Sat, May 18, 2019 at 12:02:55PM +0200, James Darnley wrote: > >> On 2019-05-18 09:39, Michael Niedermayer wrote: > >>> Fixes: "null pointer dereference" > >>> Fixes: 14551/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_V210_fuzzer-5088609952071680 > >>> > >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >>> --- > >>> libavcodec/v210dec.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/libavcodec/v210dec.c b/libavcodec/v210dec.c > >>> index bc1e1d34ff..5a33d8c089 100644 > >>> --- a/libavcodec/v210dec.c > >>> +++ b/libavcodec/v210dec.c > >>> @@ -104,7 +104,7 @@ static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, > >>> && avpkt->size - 64 >= stride * avctx->height) > >>> psrc += 64; > >>> > >>> - aligned_input = !((uintptr_t)psrc & 0xf) && !(stride & 0xf); > >>> + aligned_input = !((uintptr_t)psrc & 0x1f) && !(stride & 0x1f); > >>> if (aligned_input != s->aligned_input) { > >>> s->aligned_input = aligned_input; > >>> ff_v210dec_init(s); > >>> > >> > >> Ah yes, that'll be needed after the recent addition of avx2. LGTM and > >> sorry. > >> > > > >> I object to the commit message though because it isn't a "null pointer > >> dereference" but if that is the error as reported by the tool then keep > >> it as is. > > > > yes, the tool(s) say things like "Null-dereference READ", "SEGV on unknown address 0x000000000000" > > Is that a tool bug or are we missing something that goes on here besides the alignment? > If it's a tool bug can we try to get it fixed? > This has a big risk of wasting time if they print straight-up wrong information... I do not know for 100% sure. I actually have to admit that i tested on a pre AVX2 CPU (sandybridge). I confirmed the bug and fix on a AVX2 box but the ossfuzz stuff is not setup on that box. What ossfuzz on the web interface says is that theres a null pointer dereference in ff_v210_planar_unpack_aligned_avx2. And no anomaly is reproducable on sandybridge. The tools seem to lack support for providing usefull information about asm code. Theres for example no line numbers, no register values, ... Ill try to report this issue in case this patch fixes it (and thus proofs that the alignment fix indeed matches the null pointer report. Thanks PS: yes i know i should upgrade that sandybridge box, i had planed to but theres always something that causes me to move it down on my todo list ...
On Sat, May 18, 2019 at 01:26:01PM +0200, James Darnley wrote: > On 2019-05-18 12:15, Michael Niedermayer wrote: > > On Sat, May 18, 2019 at 12:02:55PM +0200, James Darnley wrote: > >> I object to the commit message though because it isn't a "null pointer > >> dereference" but if that is the error as reported by the tool then keep > >> it as is. > > > > yes, the tool(s) say things like "Null-dereference READ", "SEGV on unknown address 0x000000000000" > > > > Hm. It is almost certainly an aligned move on an unaligned address. > > I don't care that much about the rest of the commit message; the subject > is correct which is good enough. ok, will apply with a more clear reference to the null thanks [...]
On Sat, May 18, 2019 at 10:32:42PM +0200, Michael Niedermayer wrote: > On Sat, May 18, 2019 at 01:05:01PM +0200, Reimar Döffinger wrote: > > On 18.05.2019, at 12:15, Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > On Sat, May 18, 2019 at 12:02:55PM +0200, James Darnley wrote: > > >> On 2019-05-18 09:39, Michael Niedermayer wrote: > > >>> Fixes: "null pointer dereference" > > >>> Fixes: 14551/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_V210_fuzzer-5088609952071680 > > >>> > > >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > >>> --- > > >>> libavcodec/v210dec.c | 2 +- > > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > > >>> > > >>> diff --git a/libavcodec/v210dec.c b/libavcodec/v210dec.c > > >>> index bc1e1d34ff..5a33d8c089 100644 > > >>> --- a/libavcodec/v210dec.c > > >>> +++ b/libavcodec/v210dec.c > > >>> @@ -104,7 +104,7 @@ static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, > > >>> && avpkt->size - 64 >= stride * avctx->height) > > >>> psrc += 64; > > >>> > > >>> - aligned_input = !((uintptr_t)psrc & 0xf) && !(stride & 0xf); > > >>> + aligned_input = !((uintptr_t)psrc & 0x1f) && !(stride & 0x1f); > > >>> if (aligned_input != s->aligned_input) { > > >>> s->aligned_input = aligned_input; > > >>> ff_v210dec_init(s); > > >>> > > >> > > >> Ah yes, that'll be needed after the recent addition of avx2. LGTM and > > >> sorry. > > >> > > > > > >> I object to the commit message though because it isn't a "null pointer > > >> dereference" but if that is the error as reported by the tool then keep > > >> it as is. > > > > > > yes, the tool(s) say things like "Null-dereference READ", "SEGV on unknown address 0x000000000000" > > > > Is that a tool bug or are we missing something that goes on here besides the alignment? > > If it's a tool bug can we try to get it fixed? > > This has a big risk of wasting time if they print straight-up wrong information... > > I do not know for 100% sure. I actually have to admit that i tested on a > pre AVX2 CPU (sandybridge). I confirmed the bug and fix on a AVX2 box but > the ossfuzz stuff is not setup on that box. > What ossfuzz on the web interface says is that theres a null pointer > dereference in ff_v210_planar_unpack_aligned_avx2. And no anomaly is > reproducable on sandybridge. > The tools seem to lack support for providing usefull information about asm > code. Theres for example no line numbers, no register values, ... > Ill try to report this issue in case this patch fixes it (and thus proofs > that the alignment fix indeed matches the null pointer report. reported: https://github.com/google/oss-fuzz/issues/2465 [...]
diff --git a/libavcodec/v210dec.c b/libavcodec/v210dec.c index bc1e1d34ff..5a33d8c089 100644 --- a/libavcodec/v210dec.c +++ b/libavcodec/v210dec.c @@ -104,7 +104,7 @@ static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, && avpkt->size - 64 >= stride * avctx->height) psrc += 64; - aligned_input = !((uintptr_t)psrc & 0xf) && !(stride & 0xf); + aligned_input = !((uintptr_t)psrc & 0x1f) && !(stride & 0x1f); if (aligned_input != s->aligned_input) { s->aligned_input = aligned_input; ff_v210dec_init(s);
Fixes: "null pointer dereference" Fixes: 14551/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_V210_fuzzer-5088609952071680 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/v210dec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)