Skip to content

Handle NULL buffer when discarding rows #182

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

kornelski
Copy link
Contributor

I've got a report about a crash:

mozilla/mozjpeg#268

I've handled in the function that crashed (jquant1.c) as well as up the stack in the function that was the real cause of it (read_and_discard_scanlines uses NULL buffer and expects that to be handled gracefully).

@@ -132,6 +132,11 @@ post_process_1pass (j_decompress_ptr cinfo,
my_post_ptr post = (my_post_ptr) cinfo->post;
JDIMENSION num_rows, max_rows;

/* read_and_discard_scanlines may call it with rows "available", but no buffer */
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems kinda iffy of read_and_discard_scanlines. Is it actually intended to do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is iffy, but I don't know that function. I suppose NULL works well for discarding :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this, it seems like it's a bit of a hack anyway, so I guess the check is OK. :/

jdpostct.c Outdated
@@ -132,6 +132,11 @@ post_process_1pass (j_decompress_ptr cinfo,
my_post_ptr post = (my_post_ptr) cinfo->post;
JDIMENSION num_rows, max_rows;

/* read_and_discard_scanlines may call it with rows "available", but no buffer */
if (!output_buf) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: libjpeg style is to use (buf == NULL) (probably because of some mythical system where this will fail due to NULL being implementation defined as a a type of zero value).

jquant1.c Outdated
@@ -531,6 +531,10 @@ quantize_ord_dither (j_decompress_ptr cinfo, JSAMPARRAY input_buf,
JDIMENSION col;
JDIMENSION width = cinfo->output_width;

if (!output_buf && num_rows) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

@dcommander
Copy link
Member

@mattsarett any comments?

@@ -531,6 +531,10 @@ quantize_ord_dither (j_decompress_ptr cinfo, JSAMPARRAY input_buf,
JDIMENSION col;
JDIMENSION width = cinfo->output_width;

if (output_buf == NULL && num_rows) {
ERREXIT(cinfo, JERR_BAD_PARAM);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see JERR_BAD_PARAM defined, am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I forgot to include it in the patch. Fixed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@pgajdos
Copy link

pgajdos commented Oct 12, 2017

@kornelski
Copy link
Contributor Author

I'm not sure if that question was for me, but: this PR fixes the issue in the Suse bugzilla. It should be compatible with 1.5.2 release branch, and probably should be cherry-picked to that branch too.

@pgajdos
Copy link

pgajdos commented Oct 13, 2017

@pornel, apologize I had not been clear.

When I apply this patch to 1.5.2, the command does not crash anymore for the first test case. For the second test case though, it still crashes with the backtrace I referenced.

@kornelski
Copy link
Contributor Author

I see. I'll have a look.

@pgajdos
Copy link

pgajdos commented Oct 13, 2017

Thanks, let me know if I can help.

@kornelski
Copy link
Contributor Author

@pgajdos The second bug is worse. I've opened another issue for it #185

@dcommander
Copy link
Member

Hi, guys. I didn't write the feature in question, and apparently the author (a former Google employee) is now working elsewhere. I have a grant from Mozilla that I can use to look into this, but I need to clear some other things from my plate first. It seems like I'm going to need to really dig into the issue and re-examine some aspects of this feature, and probably design better unit tests to guard against this sort of thing in the future. I'll keep you posted.

@pgajdos
Copy link

pgajdos commented Oct 16, 2017

Thank you, guys.

@dcommander
Copy link
Member

dcommander commented Nov 14, 2017

Your patch was unfortunately insufficient to address all facets of the bug. Now that I have partial image decompression working with GIF output files, I was able to reproduce this bug with all of the various color quantization functions by passing various switches to djpeg. The root cause is that jpeg_skip_scanlines() was not properly handling color quantization. It needed to set up a dummy color_quantize() function just like it sets up a dummy color_convert() function. 5bc43c7 fixes the issue and also fixes #185 the right way, by establishing a new method in the djpeg_dest_mgr object that recomputes the buffer dimensions for output formats that work with -crop. BMP and RLE still cannot work with -crop, unfortunately, because those formats use a whole-image inversion buffer to write the scanlines in bottom-up order. However, having GIF support for the partial image decompression feature is useful, because it makes it easy to reproduce this bug. The following script should complete successfully if the bug is fully fixed:

#!/bin/bash
set -e

SRCDIR=..
BUILDDIR=.

for onepass in -onepass ""; do
	for format in gif targa; do
		for dither in fs ordered none; do
			for grayscale in -grayscale ""; do
				echo $BUILDDIR/djpeg -dct int -colors 256 -crop 98x98+12+12 $onepass -$format -dither $dither $grayscale -outfile out$onepass$grayscale-$dither.$format $SRCDIR/testimages/testorig.jpg
				$BUILDDIR/djpeg -dct int -colors 256 -crop 98x98+12+12 $onepass -$format -dither $dither $grayscale -outfile out$onepass$grayscale-$dither.$format $SRCDIR/testimages/testorig.jpg
			done
		done
	done
done

cat <<EOF >$BUILDDIR/md5sum.txt
0e36d86afcd4eba7087dcf9764fcc7bc  out-onepass-grayscale-fs.gif
4ebda254d42cbdb29c878f96b5aef3f6  out-onepass-fs.gif
c28e92b57a9d63d1f8d32f396389ca29  out-onepass-grayscale-ordered.gif
9f1c31949ed3479907775f4abc6295ec  out-onepass-ordered.gif
c28e92b57a9d63d1f8d32f396389ca29  out-onepass-grayscale-none.gif
9a18b6bbb5eeab29cd5404e755268269  out-onepass-none.gif
0c2e1ef486b21853dc670e65f53a364a  out-onepass-grayscale-fs.targa
75d8a0cf3990fec306faed32dcd8957d  out-onepass-fs.targa
82cdc85ca549e741e73a29b507cf0978  out-onepass-grayscale-ordered.targa
d8e7b513810034172c4ce5a9e6c6d3d3  out-onepass-ordered.targa
82cdc85ca549e741e73a29b507cf0978  out-onepass-grayscale-none.targa
0201ff3e239a93d12c5d7cafdec594db  out-onepass-none.targa
0e36d86afcd4eba7087dcf9764fcc7bc  out-grayscale-fs.gif
e6a7dbbcd3e176f818837b9050e25e15  out-fs.gif
c28e92b57a9d63d1f8d32f396389ca29  out-grayscale-ordered.gif
e6a7dbbcd3e176f818837b9050e25e15  out-ordered.gif
c28e92b57a9d63d1f8d32f396389ca29  out-grayscale-none.gif
741f3cf5fde61737729c13369e5845e3  out-none.gif
0c2e1ef486b21853dc670e65f53a364a  out-grayscale-fs.targa
1337a8c2d91658a10e98f4729cd4717f  out-fs.targa
82cdc85ca549e741e73a29b507cf0978  out-grayscale-ordered.targa
1337a8c2d91658a10e98f4729cd4717f  out-ordered.targa
82cdc85ca549e741e73a29b507cf0978  out-grayscale-none.targa
f0e89205504cca725b936a4809b86fcf  out-none.targa
EOF

md5sum -c $BUILDDIR/md5sum.txt

echo GREAT SUCCESS!

@kornelski kornelski deleted the quantnull branch November 14, 2017 13:29
dcommander added a commit that referenced this pull request Jul 28, 2020
- Introduce a partial image decompression regression test script that
  validates the correctness of jpeg_skip_scanlines() and
  jpeg_crop_scanlines() for a variety of cropping regions and libjpeg
  settings.

  This regression test catches the following issues:
  #182, fixed in 5bc43c7
  #237, fixed in 6e95c08
  #244, fixed in 398c1e9
  #441, fully fixed in this commit

  It does not catch the following issues:
  #194, fixed in 773040f
  #244 (additional segfault), fixed in
       9120a24

- Modify the libjpeg-turbo regression test suite (make test) so that it
  checks for the issue reported in #441 (segfault in
  jpeg_skip_scanlines() when used with 4:2:0 merged upsampling/color
  conversion.)

- Fix issues in jpeg_skip_scanlines() that caused incorrect output with
  h2v2 (4:2:0) merged upsampling/color conversion.  The previous commit
  fixed the segfault reported in #441, but that was a symptom of a
  larger problem.  Because merged 4:2:0 upsampling uses a "spare row"
  buffer, it is necessary to allow the upsampler to run when skipping
  rows (fancy 4:2:0 upsampling, which uses context rows, also requires
  this.)  Otherwise, if skipping starts at an odd-numbered row, the
  output image will be incorrect.

- Throw an error if jpeg_skip_scanlines() is called with two-pass color
  quantization enabled.  With two-pass color quantization, the first
  pass occurs within jpeg_start_decompress(), so subsequent calls to
  jpeg_skip_scanlines() interfere with the multipass state and prevent
  the second pass from occurring during subsequent calls to
  jpeg_read_scanlines().
dcommander added a commit that referenced this pull request Jul 28, 2020
- Introduce a partial image decompression regression test script that
  validates the correctness of jpeg_skip_scanlines() and
  jpeg_crop_scanlines() for a variety of cropping regions and libjpeg
  settings.

  This regression test catches the following issues:
  #182, fixed in 5bc43c7
  #237, fixed in 6e95c08
  #244, fixed in 398c1e9
  #441, fully fixed in this commit

  It does not catch the following issues:
  #194, fixed in 773040f
  #244 (additional segfault), fixed in
       9120a24

- Modify the libjpeg-turbo regression test suite (make test) so that it
  checks for the issue reported in #441 (segfault in
  jpeg_skip_scanlines() when used with 4:2:0 merged upsampling/color
  conversion.)

- Fix issues in jpeg_skip_scanlines() that caused incorrect output with
  h2v2 (4:2:0) merged upsampling/color conversion.  The previous commit
  fixed the segfault reported in #441, but that was a symptom of a
  larger problem.  Because merged 4:2:0 upsampling uses a "spare row"
  buffer, it is necessary to allow the upsampler to run when skipping
  rows (fancy 4:2:0 upsampling, which uses context rows, also requires
  this.)  Otherwise, if skipping starts at an odd-numbered row, the
  output image will be incorrect.

- Throw an error if jpeg_skip_scanlines() is called with two-pass color
  quantization enabled.  With two-pass color quantization, the first
  pass occurs within jpeg_start_decompress(), so subsequent calls to
  jpeg_skip_scanlines() interfere with the multipass state and prevent
  the second pass from occurring during subsequent calls to
  jpeg_read_scanlines().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants