Skip to content

BUG: fixed undefined behaviour with corrupted JPEG file #2933

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.

Sign up for GitHub

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

Merged
hjmjohnson merged 1 commit into from
Dec 14, 2021
Merged

BUG: fixed undefined behaviour with corrupted JPEG file #2933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Modules/IO/JPEG/include/itkJPEGImageIO.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ class ITKIOJPEG_EXPORT JPEGImageIO : public ImageIOBase
PrintSelf(std::ostream & os, Indent indent) const override;

void
WriteSlice(std::string & fileName, const void * buffer);
WriteSlice(std::string & fileName, const void * const buffer);
Comment thread
dzenanz marked this conversation as resolved.

/** Default = true*/
bool m_Progressive;
Expand Down
80 changes: 37 additions & 43 deletions Modules/IO/JPEG/src/itkJPEGImageIO.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@
#include "itk_jpeg.h"
#include <csetjmp>

#define JPEGIO_JPEG_MESSAGES 1

#if (defined JPEGIO_JPEG_MESSAGES && JPEGIO_JPEG_MESSAGES == 1)
# include <cstdio>
#endif

// create an error handler for jpeg that
// can longjmp out of the jpeg library
struct itk_jpeg_error_mgr
Expand All @@ -36,37 +42,31 @@ extern "C"
{
/* cinfo->err really points to a itk_jpeg_error_mgr struct, so coerce pointer
*/
auto * myerr = reinterpret_cast<itk_jpeg_error_mgr *>(cinfo->err);
itk_jpeg_error_mgr * myerr = (itk_jpeg_error_mgr *)cinfo->err;
Comment thread
dzenanz marked this conversation as resolved.

/* Always display the message. */
/* We could postpone this until after returning, if we chose. */
(*cinfo->err->output_message)(cinfo);

jpeg_abort(cinfo); /* clean up libjpeg state */
/* Return control to the setjmp point */
longjmp(myerr->setjmp_buffer, 1);
}

METHODDEF(void) itk_jpeg_output_message(j_common_ptr) {}
METHODDEF(void) itk_jpeg_output_message(j_common_ptr cinfo)
{
#if (defined JPEGIO_JPEG_MESSAGES && JPEGIO_JPEG_MESSAGES == 1)
char buffer[JMSG_LENGTH_MAX + 1];
(*cinfo->err->format_message)(cinfo, buffer);
printf("%s\n", buffer);
Copy link
Copy Markdown
Member

@jhlegarreta jhlegarreta Dec 12, 2021

Choose a reason for hiding this comment

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

I'd say that printing messages should be done through itkDebugMacro macro calls instead of prints. But have not checked/tested in which circumstances the message should be printed.

Copy link
Copy Markdown
Member

@issakomi issakomi Dec 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

itkDebugMacro is C++ code (std::ostringstream itkmsg), i would prefer to avoid C++ code here in extern "C", it is callback for JPEG library. Also name of function and line number are not informative in particular case.

Copy link
Copy Markdown
Member

@issakomi issakomi Dec 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

But have not checked/tested in which circumstances the message should be printed.

The image used in test:

Corrupt JPEG data: premature end of data segment
Unsupported marker type 0x02

2021-12-12T14:24:25.2286750Z 988: Test command: /home/vsts/work/1/s-build/bin/ITKIOJPEGTestDriver "itkJPEGImageIODegenerateCasesTest" "/home/vsts/work/1/s-build/ExternalData/Modules/IO/JPEG/test/Input/corrupted_image.jpg"
2021-12-12T14:24:25.2287348Z 988: Test timeout computed to be: 1500
2021-12-12T14:24:25.2807228Z 988: Trying reader->Update()
2021-12-12T14:24:25.2878165Z 988: Corrupt JPEG data: premature end of data segment
2021-12-12T14:24:25.2879804Z 988: Unsupported marker type 0x02
2021-12-12T14:24:25.2880768Z 988: WARNING: In /home/vsts/work/1/s/Modules/IO/JPEG/src/itkJPEGImageIO.cxx, line 223
2021-12-12T14:24:25.2882128Z 988: JPEGImageIO (0x564bc75c26b0): JPEG error in the file /home/vsts/work/1/s-build/ExternalData/Modules/IO/JPEG/test/Input/corrupted_image.jpg
2021-12-12T14:24:25.2882707Z 988: 
2021-12-12T14:24:25.2882965Z 988: Test finished.

The second (triggers exception) from the post

Corrupt JPEG data: 152 extraneous bytes before marker 0xda
Unsupported marker type 0x02

This images https://drive.google.com/file/d/1zEIEtpDai_FUtChhm1WQR_eUuePbmSVB/view?usp=sharing
doesn't trigger jump, only internal JPEG message

Corrupt JPEG data: premature end of data segment

#else
(void)cinfo;
#endif
}
}

namespace itk
{

namespace
{
// Wrap setjmp call to avoid warnings about variable clobbering.
bool
wrapSetjmp(itk_jpeg_error_mgr & jerr)
{
if (setjmp(jerr.setjmp_buffer))
{
return true;
}
return false;
}
} // namespace

// simple class to call fopen on construct and
// fclose on destruct
class JPEGFileWrapper
Expand All @@ -86,7 +86,7 @@ class JPEGFileWrapper
}
}

FILE * m_FilePointer;
FILE * volatile m_FilePointer;
};

bool
Expand Down Expand Up @@ -142,7 +142,7 @@ JPEGImageIO::CanReadFile(const char * file)
jerr.pub.output_message = itk_jpeg_output_message;
// set the jump point, if there is a jpeg error or warning
// this will evaluate to true
if (wrapSetjmp(jerr))
if (setjmp(jerr.setjmp_buffer))
{
// clean up
jpeg_destroy_decompress(&cinfo);
Expand All @@ -166,13 +166,9 @@ void
JPEGImageIO::ReadVolume(void *)
{}

//-----------------------------------------------------------------------------

void
JPEGImageIO::Read(void * buffer)
{
unsigned int ui;

// use this class so return will call close
JPEGFileWrapper JPEGfp(this->GetFileName(), "rb");
FILE * fp = JPEGfp.m_FilePointer;
Expand All @@ -193,7 +189,7 @@ JPEGImageIO::Read(void * buffer)
jerr.pub.error_exit = itk_jpeg_error_exit;
// for any output message call itk_jpeg_output_message
jerr.pub.output_message = itk_jpeg_output_message;
if (wrapSetjmp(jerr))
if (setjmp(jerr.setjmp_buffer))
{
// clean up
jpeg_destroy_decompress(&cinfo);
Expand All @@ -212,21 +208,26 @@ JPEGImageIO::Read(void * buffer)
// prepare to read the bulk data
jpeg_start_decompress(&cinfo);

SizeValueType rowbytes = cinfo.output_components * cinfo.output_width;
auto * tempImage = static_cast<JSAMPLE *>(buffer);
const auto rowbytes = cinfo.output_width * cinfo.output_components;
auto * tempImage = static_cast<JSAMPLE *>(buffer);

auto * row_pointers = new JSAMPROW[cinfo.output_height];
for (ui = 0; ui < cinfo.output_height; ++ui)
auto * volatile row_pointers = new JSAMPROW[cinfo.output_height];
for (size_t ui = 0; ui < cinfo.output_height; ++ui)
{
row_pointers[ui] = tempImage + rowbytes * ui;
}

// read the bulk data
unsigned int remainingRows;
while (cinfo.output_scanline < cinfo.output_height)
{
remainingRows = cinfo.output_height - cinfo.output_scanline;
jpeg_read_scanlines(&cinfo, &row_pointers[cinfo.output_scanline], remainingRows);
if (setjmp(jerr.setjmp_buffer))
{
itkWarningMacro("JPEG error in the file " << this->GetFileName());
Comment thread
dzenanz marked this conversation as resolved.
jpeg_destroy_decompress(&cinfo);
delete[] row_pointers;
return;
}
jpeg_read_scanlines(&cinfo, &row_pointers[cinfo.output_scanline], cinfo.output_height - cinfo.output_scanline);
}

// finish the decompression step
Expand Down Expand Up @@ -398,7 +399,7 @@ JPEGImageIO::Write(const void * buffer)
}

void
JPEGImageIO::WriteSlice(std::string & fileName, const void * buffer)
JPEGImageIO::WriteSlice(std::string & fileName, const void * const buffer)
{
// use this class so return will call close
JPEGFileWrapper JPEGfp(fileName.c_str(), "wb");
Expand All @@ -410,22 +411,14 @@ JPEGImageIO::WriteSlice(std::string & fileName, const void * buffer)
<< "Reason: " << itksys::SystemTools::GetLastSystemError());
}

// Call the correct templated function for the output

// overriding jpeg_error_mgr so we don't exit when an error happens
// Create the jpeg compression object and error handler
// struct jpeg_compress_struct cinfo;
// struct itk_jpeg_error_mgr jerr;

struct itk_jpeg_error_mgr jerr;
struct jpeg_compress_struct cinfo;
cinfo.err = jpeg_std_error(&jerr.pub);
// set the jump point, if there is a jpeg error or warning
// this will evaluate to true
if (wrapSetjmp(jerr))
// set the jump point
if (setjmp(jerr.setjmp_buffer))
{
jpeg_destroy_compress(&cinfo);
itkExceptionMacro(<< "JPEG : Out of disk space");
itkExceptionMacro(<< "JPEG error, failed to write " << fileName);
}

jpeg_create_compress(&cinfo);
Expand Down Expand Up @@ -526,6 +519,7 @@ JPEGImageIO::WriteSlice(std::string & fileName, const void * buffer)

if (fflush(fp) == EOF)
{
delete[] row_pointers;
itkExceptionMacro(<< "JPEG : Out of disk space");
}

Expand Down
4 changes: 4 additions & 0 deletions Modules/IO/JPEG/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ itk_module_test()
set(ITKIOJPEGTests
itkJPEGImageIOTest.cxx
itkJPEGImageIOTest2.cxx
itkJPEGImageIODegenerateCasesTest.cxx
)

CreateTestDriver(ITKIOJPEG "${ITKIOJPEG-Test_LIBRARIES}" "${ITKIOJPEGTests}")
Expand All @@ -19,3 +20,6 @@ itk_add_test(NAME itkJPEGImageIOTest2
itk_add_test(NAME itkJPEGImageIOSpacing
COMMAND ITKIOJPEGTestDriver
itkJPEGImageIOTest2 ${ITK_TEST_OUTPUT_DIR}/itkJPEGImageIOSpacing.jpg)
itk_add_test(NAME itkJPEGImageIOTestCorruptedImage
COMMAND ITKIOJPEGTestDriver
itkJPEGImageIODegenerateCasesTest DATA{Input/corrupted_image.jpg})
1 change: 1 addition & 0 deletions Modules/IO/JPEG/test/Input/corrupted_image.jpg.sha512
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
c97188b2a9c38dae4e8a40ed6e533cb3380775d87431c97e3cc92a3dd80215c6b37fffc9db855ef22e940497027a873cdff4f191fa40236eafc217fb32e994cd
52 changes: 52 additions & 0 deletions Modules/IO/JPEG/test/itkJPEGImageIODegenerateCasesTest.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*=========================================================================
*
* Copyright NumFOCUS
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0.txt
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*=========================================================================*/

#include "itkJPEGImageIO.h"
#include "itkImageFileReader.h"
#include "itkTestingMacros.h"


int
itkJPEGImageIODegenerateCasesTest(int argc, char * argv[])
{
if (argc != 2)
{
std::cerr << "Missing parameters." << std::endl;
std::cerr << "Usage: " << itkNameOfTestExecutableMacro(argv);
std::cerr << " inputFilename" << std::endl;
return EXIT_FAILURE;
}

constexpr unsigned int Dimension = 2;
using PixelType = unsigned char;

using ImageType = itk::Image<PixelType, Dimension>;

itk::JPEGImageIO::Pointer io = itk::JPEGImageIO::New();

itk::ImageFileReader<ImageType>::Pointer reader = itk::ImageFileReader<ImageType>::New();

reader->SetFileName(argv[1]);
reader->SetImageIO(io);

ITK_TRY_EXPECT_NO_EXCEPTION(reader->Update());
Copy link
Copy Markdown
Member

@issakomi issakomi Dec 12, 2021

Choose a reason for hiding this comment

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

@jhlegarreta This particular file doesn't trigger exception, so the test works, other broken files can trigger exception, before entering the scan-lines while loop. Important is to avoid segmentation fault or crash, exception is OK. Just FYI.

Copy link
Copy Markdown
Member

@jhlegarreta jhlegarreta Dec 12, 2021

Choose a reason for hiding this comment

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

Hats off to the effort to investigate this @issakomi. I cannot investigate as closely. I can only point to a few things:

  • I am not sure about the debug/release compilation of the CI builds, so it might well be that the image provided by @Hconk does not trigger any issue in our CI builds. However, many CDash builds are build in release mode, so any (appropriately) corrupted file without this fix should trigger the issue as I understand it.
  • If prior to this patch set a given test file was generating a segmentation fault or crash of any nature, then that test file should be added to the test images, and ensure that it does not produce a segfault with this patch set, and is dealt with nicely, either being read if it can be read or generating an exception (and I'd say exception and not warning) saying that it cannot be read. That exception can be tested with the ITK_TRY_EXPECT_EXCEPTION macro.
  • If the image provided by @Hconk is now being dealt with nicely and can be read without issues, the ITK_TRY_EXPECT_NO_EXCEPTION is OK.
  • Otherwise, I do not see that this patch set adds new exceptions. If any of the existing ones were not being tested, and we would like them to be tested, images triggering them should be added. See here the exceptions that are not being exercised (any line in red is not exercised/tested):
    https://open.cdash.org/viewCoverageFile.php?buildid=7620043&fileid=49347999

Hope this casts some light on what is desired to fully test these cases, and to ensure that the behavior is predictable and conforming to the expectations.

Copy link
Copy Markdown
Member

@issakomi issakomi Dec 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The issue with undefined behavior is fixed, incorrect setjmp implementation caused it. It works in debug, release etc. the same way.

See here the exceptions that are not being exercised (any line in red is not exercised/tested):
https://open.cdash.org/viewCoverageFile.php?buildid=7620043&fileid=49347999

I can give you an image that will trigger exception. This one, for example.
https://drive.google.com/file/d/1zGobe2UkAo1nEYMfEv5IV1TM5bhMYx7G/view?usp=sharing

Copy link
Copy Markdown
Member

@jhlegarreta jhlegarreta Dec 12, 2021

Choose a reason for hiding this comment

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

OK. Thanks. Then testing the exceptions would be best addressed in a separate PR. I will keep a note and propose a separate PR that uses the shared image (thanks for doing it) as time permits.



std::cout << "Test finished." << std::endl;
return EXIT_SUCCESS;
}