| Message ID | 20170904220408.27284-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
Hi, On Mon, Sep 4, 2017 at 6:04 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > Fixes: OOM > Fixes: 2225/clusterfuzz-testcase-minimized-5505632079708160 > > Found-by: continuous fuzzing process https://github.com/google/oss- > fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/jpeg2000.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/libavcodec/jpeg2000.c b/libavcodec/jpeg2000.c > index 94efc94c4d..9e1bbc2ec4 100644 > --- a/libavcodec/jpeg2000.c > +++ b/libavcodec/jpeg2000.c > @@ -506,6 +506,10 @@ int ff_jpeg2000_init_component(Jpeg2000Component > *comp, > // update precincts size: 2^n value > reslevel->log2_prec_width = codsty->log2_prec_widths[ > reslevelno]; > reslevel->log2_prec_height = codsty->log2_prec_heights[ > reslevelno]; > + if (!reslevel->log2_prec_width || !reslevel->log2_prec_height) { > + av_log(avctx, AV_LOG_ERROR, "COD/COC is missing\n"); > + return AVERROR_INVALIDDATA; > + } Please change it to ff_tlog(). Ronald
On Tue, 5 Sep 2017 00:04:08 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > Fixes: OOM > Fixes: 2225/clusterfuzz-testcase-minimized-5505632079708160 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/jpeg2000.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/libavcodec/jpeg2000.c b/libavcodec/jpeg2000.c > index 94efc94c4d..9e1bbc2ec4 100644 > --- a/libavcodec/jpeg2000.c > +++ b/libavcodec/jpeg2000.c > @@ -506,6 +506,10 @@ int ff_jpeg2000_init_component(Jpeg2000Component *comp, > // update precincts size: 2^n value > reslevel->log2_prec_width = codsty->log2_prec_widths[reslevelno]; > reslevel->log2_prec_height = codsty->log2_prec_heights[reslevelno]; > + if (!reslevel->log2_prec_width || !reslevel->log2_prec_height) { > + av_log(avctx, AV_LOG_ERROR, "COD/COC is missing\n"); > + return AVERROR_INVALIDDATA; > + } > > /* Number of bands for each resolution level */ > if (reslevelno == 0) Just after we've _extensively_ discussed how such cryptic messages are a bad idea, you're back with a new patch following exactly the same pattern.
Hi On Mon, Sep 04, 2017 at 06:45:02PM -0400, Ronald S. Bultje wrote: > Hi, > > On Mon, Sep 4, 2017 at 6:04 PM, Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > Fixes: OOM > > Fixes: 2225/clusterfuzz-testcase-minimized-5505632079708160 > > > > Found-by: continuous fuzzing process https://github.com/google/oss- > > fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/jpeg2000.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/libavcodec/jpeg2000.c b/libavcodec/jpeg2000.c > > index 94efc94c4d..9e1bbc2ec4 100644 > > --- a/libavcodec/jpeg2000.c > > +++ b/libavcodec/jpeg2000.c > > @@ -506,6 +506,10 @@ int ff_jpeg2000_init_component(Jpeg2000Component > > *comp, > > // update precincts size: 2^n value > > reslevel->log2_prec_width = codsty->log2_prec_widths[ > > reslevelno]; > > reslevel->log2_prec_height = codsty->log2_prec_heights[ > > reslevelno]; > > + if (!reslevel->log2_prec_width || !reslevel->log2_prec_height) { > > + av_log(avctx, AV_LOG_ERROR, "COD/COC is missing\n"); > > + return AVERROR_INVALIDDATA; > > + } > > > Please change it to ff_tlog(). that would make the message unavailable to the user, so the user would not know why a decoding failure occured. It would also make it unavailable in bug reports as the message is not in the compiled binary. Even at highest verbosity and debug levels it would not show up not even with debug builds. Only in special trace builds would it show up. Users would not be able to find existing bug reports based on the error message, would not be able to google it, would not be able to refer to it in a specific way "a issue with missing COC/COD". This is not a obscure detail of bitstream parsing, its a error in the headers that will lead to the loss of a frame. Lets also look at what other software does picking lena converted to jpeg2000 and a damaged COD with a hex editor j2k_to_image -i lena-noco.jp2 -o image.pgm [ERROR] Error decoding component 0. The number of resolutions is too big: 256 vs max= 33. Truncating. [ERROR] Error decoding component 1. The number of resolutions is too big: 256 vs max= 33. Truncating. [ERROR] Error decoding component 2. The number of resolutions is too big: 256 vs max= 33. Truncating. [ERROR] Failed to decode J2K image ERROR -> j2k_to_image: failed to decode image! You can see openjpeg shows detailed error messages Lets try the clusterfuzz testcase directly: j2k_to_image -i clusterfuzz-testcase-minimized-5505632079708160.jp2 -o image.pnm [ERROR] Integer overflow in box->length [ERROR] Failed to read boxhdr [ERROR] Failed to decode jp2 structure ERROR -> j2k_to_image: failed to decode image! again, a detailed error message lets try jasper jasper --input lena-noco.jp2 --output file.pnm cannot get marker segment error: cannot decode code stream error: cannot load image data and the testcase directly: jasper --input clusterfuzz-testcase-minimized-5505632079708160 --output image.pnm cannot get marker segment error: cannot load image data and jasper also shows more than just a generic error Thats by default. no debug build, no trace build, no verbosity, no debug options. just for completeness lets run jasper with debug level 99 jasper --debug-level 99 --input clusterfuzz-testcase-minimized-5505632079708160.jp2 --output image.pnm type = 0xff4f (SOC); type = 0xff51 (SIZ); len = 41;caps = 0x2020; width = 25632; height = 32; xoff = 0; yoff = 0; tilewidth = 538976288; tileheight = 538976288; tilexoff = 0; tileyoff = 0; prec[0] = 8; sgnd[0] = 0; hsamp[0] = 1; vsamp[0] = 1 type = 0xff52 (COD); len = 13;csty = 0x01; numdlvls = 0; qmfbid = 0; mctrans = 0 prg = 32; numlyrs = 8224; cblkwidthval = 32; cblkheightval = 32; cblksty = 0x20; prcwidth[0] = 0, prcheight[0] = 0 type = 0xff90 (SOT); len = 10;tileno = 0; len = 0; partno = 0; numparts = 32 cannot get marker segment error: cannot load image data You can again see, theres lots of details, which may be critically important in a bug report. More so users may have bug report samples that are not sharable for all kinds of contractual reasons. Having detailed information available is the only chance to debug such issues. Requiring the user to build his own binary FFmpeg with custom build flags is a large hurdle for reporting a bug Thanks [...]
On 9/5/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > Hi > > On Mon, Sep 04, 2017 at 06:45:02PM -0400, Ronald S. Bultje wrote: >> Hi, >> >> On Mon, Sep 4, 2017 at 6:04 PM, Michael Niedermayer >> <michael@niedermayer.cc> >> wrote: >> >> > Fixes: OOM >> > Fixes: 2225/clusterfuzz-testcase-minimized-5505632079708160 >> > >> > Found-by: continuous fuzzing process https://github.com/google/oss- >> > fuzz/tree/master/projects/ffmpeg >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> > --- >> > libavcodec/jpeg2000.c | 4 ++++ >> > 1 file changed, 4 insertions(+) >> > >> > diff --git a/libavcodec/jpeg2000.c b/libavcodec/jpeg2000.c >> > index 94efc94c4d..9e1bbc2ec4 100644 >> > --- a/libavcodec/jpeg2000.c >> > +++ b/libavcodec/jpeg2000.c >> > @@ -506,6 +506,10 @@ int ff_jpeg2000_init_component(Jpeg2000Component >> > *comp, >> > // update precincts size: 2^n value >> > reslevel->log2_prec_width = codsty->log2_prec_widths[ >> > reslevelno]; >> > reslevel->log2_prec_height = codsty->log2_prec_heights[ >> > reslevelno]; >> > + if (!reslevel->log2_prec_width || !reslevel->log2_prec_height) >> > { >> > + av_log(avctx, AV_LOG_ERROR, "COD/COC is missing\n"); >> > + return AVERROR_INVALIDDATA; >> > + } >> >> >> Please change it to ff_tlog(). > > that would make the message unavailable to the user, so the user > would not know why a decoding failure occured. > > It would also make it unavailable in bug reports as the message is > not in the compiled binary. Even at highest verbosity and debug levels > it would not show up not even with debug builds. Only in special trace > builds would it show up. > > Users would not be able to find existing bug reports based on the error > message, would not be able to google it, would not be able to refer to > it in a specific way "a issue with missing COC/COD". > > This is not a obscure detail of bitstream parsing, its a error in > the headers that will lead to the loss of a frame. > > Lets also look at what other software does > picking lena converted to jpeg2000 and a damaged COD with a hex editor > > j2k_to_image -i lena-noco.jp2 -o image.pgm > > [ERROR] Error decoding component 0. > The number of resolutions is too big: 256 vs max= 33. Truncating. > > [ERROR] Error decoding component 1. > The number of resolutions is too big: 256 vs max= 33. Truncating. > > [ERROR] Error decoding component 2. > The number of resolutions is too big: 256 vs max= 33. Truncating. > > [ERROR] Failed to decode J2K image > ERROR -> j2k_to_image: failed to decode image! > > You can see openjpeg shows detailed error messages > Lets try the clusterfuzz testcase directly: > > j2k_to_image -i clusterfuzz-testcase-minimized-5505632079708160.jp2 -o > image.pnm > > [ERROR] Integer overflow in box->length > [ERROR] Failed to read boxhdr > [ERROR] Failed to decode jp2 structure > ERROR -> j2k_to_image: failed to decode image! > > again, a detailed error message > > lets try jasper > > jasper --input lena-noco.jp2 --output file.pnm > cannot get marker segment > error: cannot decode code stream > error: cannot load image data > > and the testcase directly: > jasper --input clusterfuzz-testcase-minimized-5505632079708160 --output > image.pnm > cannot get marker segment > error: cannot load image data > > and jasper also shows more than just a generic error > > Thats by default. no debug build, no trace build, no verbosity, no > debug options. > > just for completeness lets run jasper with debug level 99 > jasper --debug-level 99 --input > clusterfuzz-testcase-minimized-5505632079708160.jp2 --output image.pnm > type = 0xff4f (SOC); > type = 0xff51 (SIZ); len = 41;caps = 0x2020; > width = 25632; height = 32; xoff = 0; yoff = 0; > tilewidth = 538976288; tileheight = 538976288; tilexoff = 0; tileyoff = > 0; > prec[0] = 8; sgnd[0] = 0; hsamp[0] = 1; vsamp[0] = 1 > type = 0xff52 (COD); len = 13;csty = 0x01; > numdlvls = 0; qmfbid = 0; mctrans = 0 > prg = 32; numlyrs = 8224; > cblkwidthval = 32; cblkheightval = 32; cblksty = 0x20; > prcwidth[0] = 0, prcheight[0] = 0 > type = 0xff90 (SOT); len = 10;tileno = 0; len = 0; partno = 0; numparts > = 32 > cannot get marker segment > error: cannot load image data > > You can again see, theres lots of details, which may be critically > important in a bug report. > > More so users may have bug report samples that are not sharable > for all kinds of contractual reasons. Having detailed information > available is the only chance to debug such issues. > Requiring the user to build his own binary FFmpeg with custom build > flags is a large hurdle for reporting a bug FFmpeg is not meant to have every log for every error return. COD/COC sounds funny. Thanks
Hi, This really isn't worth our time. Really, it isn't. On Tue, Sep 5, 2017 at 7:04 AM, Michael Niedermayer <michael@niedermayer.cc> wrote: > > Please change it to ff_tlog(). > > that would make the message unavailable to the user, so the user > would not know why a decoding failure occured. > It is intentional to not make it available to the end user. That is by design. The message is inappropriate for end users. For example, it doesn't tell in a human-readable form what's going on or how to solve it or where to go for more information. It isn't helpful at all. I don't know a single end user who even knows what a COD is, they probably think it's a video game (try Googling it). > Lets also look at what other software does > "Mommy, he did it first." > You can again see, theres lots of details, which may be critically > important in a bug report. There are no bug reports related to this issue, otherwise we would have fixed this security issue long ago. This is a fuzz-only issue. That makes it all the more important to not waste precious bytes in our binaries on it, or lines on our commandline terminals. When I get 1000s of lines of debug output, I ignore them all unless I grep'ed for something specific to not make it 1000s. When I get 10, I might actually read the first 2. Please change the message to ff_dlog() if you really, really insist on something else than ff_tlog(). That way, our precious end users don't need to see it. Ronald
On Tue, Sep 05, 2017 at 01:16:22PM +0200, Paul B Mahol wrote: > On 9/5/17, Michael Niedermayer <michael@niedermayer.cc> wrote: > > Hi > > > > On Mon, Sep 04, 2017 at 06:45:02PM -0400, Ronald S. Bultje wrote: > >> Hi, > >> > >> On Mon, Sep 4, 2017 at 6:04 PM, Michael Niedermayer > >> <michael@niedermayer.cc> > >> wrote: > >> > >> > Fixes: OOM > >> > Fixes: 2225/clusterfuzz-testcase-minimized-5505632079708160 > >> > > >> > Found-by: continuous fuzzing process https://github.com/google/oss- > >> > fuzz/tree/master/projects/ffmpeg > >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> > --- > >> > libavcodec/jpeg2000.c | 4 ++++ > >> > 1 file changed, 4 insertions(+) > >> > > >> > diff --git a/libavcodec/jpeg2000.c b/libavcodec/jpeg2000.c > >> > index 94efc94c4d..9e1bbc2ec4 100644 > >> > --- a/libavcodec/jpeg2000.c > >> > +++ b/libavcodec/jpeg2000.c > >> > @@ -506,6 +506,10 @@ int ff_jpeg2000_init_component(Jpeg2000Component > >> > *comp, > >> > // update precincts size: 2^n value > >> > reslevel->log2_prec_width = codsty->log2_prec_widths[ > >> > reslevelno]; > >> > reslevel->log2_prec_height = codsty->log2_prec_heights[ > >> > reslevelno]; > >> > + if (!reslevel->log2_prec_width || !reslevel->log2_prec_height) > >> > { > >> > + av_log(avctx, AV_LOG_ERROR, "COD/COC is missing\n"); > >> > + return AVERROR_INVALIDDATA; > >> > + } > >> > >> > >> Please change it to ff_tlog(). > > > > that would make the message unavailable to the user, so the user > > would not know why a decoding failure occured. > > > > It would also make it unavailable in bug reports as the message is > > not in the compiled binary. Even at highest verbosity and debug levels > > it would not show up not even with debug builds. Only in special trace > > builds would it show up. > > > > Users would not be able to find existing bug reports based on the error > > message, would not be able to google it, would not be able to refer to > > it in a specific way "a issue with missing COC/COD". > > > > This is not a obscure detail of bitstream parsing, its a error in > > the headers that will lead to the loss of a frame. > > > > Lets also look at what other software does > > picking lena converted to jpeg2000 and a damaged COD with a hex editor > > > > j2k_to_image -i lena-noco.jp2 -o image.pgm > > > > [ERROR] Error decoding component 0. > > The number of resolutions is too big: 256 vs max= 33. Truncating. > > > > [ERROR] Error decoding component 1. > > The number of resolutions is too big: 256 vs max= 33. Truncating. > > > > [ERROR] Error decoding component 2. > > The number of resolutions is too big: 256 vs max= 33. Truncating. > > > > [ERROR] Failed to decode J2K image > > ERROR -> j2k_to_image: failed to decode image! > > > > You can see openjpeg shows detailed error messages > > Lets try the clusterfuzz testcase directly: > > > > j2k_to_image -i clusterfuzz-testcase-minimized-5505632079708160.jp2 -o > > image.pnm > > > > [ERROR] Integer overflow in box->length > > [ERROR] Failed to read boxhdr > > [ERROR] Failed to decode jp2 structure > > ERROR -> j2k_to_image: failed to decode image! > > > > again, a detailed error message > > > > lets try jasper > > > > jasper --input lena-noco.jp2 --output file.pnm > > cannot get marker segment > > error: cannot decode code stream > > error: cannot load image data > > > > and the testcase directly: > > jasper --input clusterfuzz-testcase-minimized-5505632079708160 --output > > image.pnm > > cannot get marker segment > > error: cannot load image data > > > > and jasper also shows more than just a generic error > > > > Thats by default. no debug build, no trace build, no verbosity, no > > debug options. > > > > just for completeness lets run jasper with debug level 99 > > jasper --debug-level 99 --input > > clusterfuzz-testcase-minimized-5505632079708160.jp2 --output image.pnm > > type = 0xff4f (SOC); > > type = 0xff51 (SIZ); len = 41;caps = 0x2020; > > width = 25632; height = 32; xoff = 0; yoff = 0; > > tilewidth = 538976288; tileheight = 538976288; tilexoff = 0; tileyoff = > > 0; > > prec[0] = 8; sgnd[0] = 0; hsamp[0] = 1; vsamp[0] = 1 > > type = 0xff52 (COD); len = 13;csty = 0x01; > > numdlvls = 0; qmfbid = 0; mctrans = 0 > > prg = 32; numlyrs = 8224; > > cblkwidthval = 32; cblkheightval = 32; cblksty = 0x20; > > prcwidth[0] = 0, prcheight[0] = 0 > > type = 0xff90 (SOT); len = 10;tileno = 0; len = 0; partno = 0; numparts > > = 32 > > cannot get marker segment > > error: cannot load image data > > > > You can again see, theres lots of details, which may be critically > > important in a bug report. > > > > More so users may have bug report samples that are not sharable > > for all kinds of contractual reasons. Having detailed information > > available is the only chance to debug such issues. > > Requiring the user to build his own binary FFmpeg with custom build > > flags is a large hurdle for reporting a bug > > FFmpeg is not meant to have every log for every error return. > > COD/COC sounds funny. COD/COC is how its called in most of the jpeg2000 spec I can write the full terms out of course "Coding style component" and "Coding style default" [...]
diff --git a/libavcodec/jpeg2000.c b/libavcodec/jpeg2000.c index 94efc94c4d..9e1bbc2ec4 100644 --- a/libavcodec/jpeg2000.c +++ b/libavcodec/jpeg2000.c @@ -506,6 +506,10 @@ int ff_jpeg2000_init_component(Jpeg2000Component *comp, // update precincts size: 2^n value reslevel->log2_prec_width = codsty->log2_prec_widths[reslevelno]; reslevel->log2_prec_height = codsty->log2_prec_heights[reslevelno]; + if (!reslevel->log2_prec_width || !reslevel->log2_prec_height) { + av_log(avctx, AV_LOG_ERROR, "COD/COC is missing\n"); + return AVERROR_INVALIDDATA; + } /* Number of bands for each resolution level */ if (reslevelno == 0)
Fixes: OOM Fixes: 2225/clusterfuzz-testcase-minimized-5505632079708160 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/jpeg2000.c | 4 ++++ 1 file changed, 4 insertions(+)