BUG: fixed undefined behaviour with corrupted JPEG file by dzenanz · Pull Request #2933 · InsightSoftwareConsortium/ITK

dzenanz · 2021-12-09T16:16:50Z

dzenanz

Mostly looks good.

jhlegarreta · 2021-12-09T17:59:49Z

Have not followed closely the issue/source of the problem, but 💯 to all involved for investigating this and proposing a fix.

Can the image file that demonstrates the issue or an appropriate one (i.e. having some compatible license) be added to the test images and be provided to the test as another case so that to ensure that the issue does not leak back into the code ?

issakomi · 2021-12-09T18:35:00Z

S. link to the file #2928
@jhlegarreta Short video:
https://youtu.be/zJf3maxYaN0

dzenanz · 2021-12-09T19:09:42Z

A warning: Modules/IO/JPEG/src/itkJPEGImageIO.cxx:160:10: warning: variable 'ui' might be clobbered by 'longjmp' or 'vfork' [-Wclobbered]

jhlegarreta · 2021-12-11T01:33:50Z

Can the image file that demonstrates the issue or an appropriate one (i.e. having some compatible license) be added to the test images and be provided to the test as another case so that to ensure that the issue does not leak back into the code ?

Here it is: issakomi#1

jhlegarreta · 2021-12-11T01:42:30Z

@Hconk hope it is OK to use the image you shared. Let us know otherwise.

issakomi · 2021-12-11T01:59:30Z

Thank you, i have merged the test. I am still working on the branch. It is WIP, please don't merge into ITK master just now.

The image is interesting. In fact if someone just cut in hex editor parts of a image, JPEG library can survive, i tried to produce corrupted images, there were only warnings. BTW, warnings are unfortunately invisible in current JPEG IO, messages are supressed.
METHODDEF(void) itk_jpeg_output_message(j_common_ptr) {}

This particular image has 2 issues

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

Don't know exactly, maybe the second causes the critical error, not 100% sure.

issakomi · 2021-12-11T05:35:59Z

To demonstrate the bug with setjmp wrapping, Debian 10, GCC 8.3

This program

#include <cstdio>
#include <csetjmp>

namespace
{
bool wrap(jmp_buf env)
{
  if (setjmp(env))
  {
    return true;
  }
  return false;
}
}

int main(int, char**)
{
  jmp_buf env;
  volatile int x = 0;
//  if (wrap(env))
  if (setjmp(env))
  {
    ++x;
    printf("%d\n", x);
  }
  longjmp(env, 1);
  return 0;
}

with if (setjmp(env)) will print incrementing number forever (caution, must terminate with Ctrl-C), jumping forever.

r@deb:~$ ./a.out 
1
2
3
4
...
^C

with if (wrap(env))

r@deb:~$ ./a.out 
Segmentation fault

Similar program

#include <cstdio>
#include <csetjmp>

namespace
{
bool wrap(jmp_buf env)
{
  if (setjmp(env))
  {
    return true;
  }
  return false;
}
}

int main(int, char**)
{
  jmp_buf env;
  volatile int x = 0;
//  if (wrap(env))
  if (setjmp(env))
  {
    ++x;
    if (x <= 5)
    {
      printf("%d\n", x);
    }
    else
    {
      printf("return point 1\n");
      return 0;
    }
  }
  longjmp(env, 1);
  printf("return point 2\n");
  return 0;
}

with if (setjmp(env))

r@deb:~$ ./a.out 
1
2
3
4
5
return point 1
r@deb:~$

with if (wrap(env))

r@deb:~$ ./a.out 
r@deb:~$

prints nothing somehow.

Hconk · 2021-12-11T06:48:04Z

@Hconk hope it is OK to use the image you shared. Let us know otherwise.

Yes，it's OK. I hava some similar images, If necessary, I will share it when I have time.

Hconk · 2021-12-11T09:13:44Z

Thank you, i have merged the test. I am still working on the branch. It is WIP, please don't merge into ITK master just now.

The image is interesting. In fact if someone just cut in hex editor parts of a image, JPEG library can survive, i tried to produce corrupted images, there were only warnings. BTW, warnings are unfortunately invisible in current JPEG IO, messages are supressed. METHODDEF(void) itk_jpeg_output_message(j_common_ptr) {}

This particular image has 2 issues
Corrupt JPEG data: premature end of data segment
Unsupported marker type 0x02
Don't know exactly, maybe the second causes the critical error, not 100% sure.

I found easy way to produce corrupted jpeg file. insert FF 02 into SOS(0xFFDA) field will cause crash. Hope to bring some help to your work.
Minimum example
good image hex:

λ Desktop itk_demo → cat ./good.jpg| xxd -u
00000000: FFD8 FFE0 0010 4A46 4946 0001 0101 0060  ......JFIF.....`
00000010: 0060 0000 FFDB 0043 0001 0101 0101 0101  .`.....C........
00000020: 0101 0101 0101 0101 0101 0101 0101 0101  ................
00000030: 0101 0101 0101 0101 0101 0101 0101 0101  ................
00000040: 0101 0101 0101 0101 0101 0101 0101 0101  ................
00000050: 0101 0101 0101 0101 01FF C900 0B08 0001  ................
00000060: 0001 0101 1100 FFDA 0008 0101 0000 3F00  ..............?.
00000070: D2CF 20FF D9                             .. ..

corrupted image hex:

λ Desktop itk_demo → cat corrupted.jpg| xxd -u
00000000: FFD8 FFE0 0010 4A46 4946 0001 0101 0060  ......JFIF.....`
00000010: 0060 0000 FFDB 0043 0001 0101 0101 0101  .`.....C........
00000020: 0101 0101 0101 0101 0101 0101 0101 0101  ................
00000030: 0101 0101 0101 0101 0101 0101 0101 0101  ................
00000040: 0101 0101 0101 0101 0101 0101 0101 0101  ................
00000050: 0101 0101 0101 0101 01FF C900 0B08 0001  ................
00000060: 0001 0101 1100 FFDA 0008 0101 0000 3F00  ..............?.
00000070: D2CF 20FF 02FF D9                        .. ....

issakomi · 2021-12-12T12:51:40Z

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


@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.

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.

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

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.

jhlegarreta · 2021-12-12T15:25:59Z

+#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);


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.

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.

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

dzenanz · 2021-12-13T16:29:52Z

I don't know whether this is ready for review and merge or not. If it is not, please convert it into a draft PR.

issakomi · 2021-12-14T02:33:00Z

I don't know whether this is ready for review and merge or not. If it is not, please convert it into a draft PR.

Yes, it is.

issakomi · 2021-12-14T08:36:29Z

Just realized that in

class JPEGFileWrapper
{
public:
  JPEGFileWrapper(const char * const fname, const char * const openMode)
    : m_FilePointer(nullptr)
  {
    m_FilePointer = fopen(fname, openMode);
  }

  virtual ~JPEGFileWrapper()
  {
    if (m_FilePointer != nullptr)
    {
      fclose(m_FilePointer);
    }
  }

  FILE * m_FilePointer;
};

FILE * m_FilePointer; probably would be better FILE * volatile m_FilePointer; The class destructor may be triggered by longjmp. Generally everything accessed during jumping should be volatile, AFAIK.

I still have doubts: do you wish JPEG messages or not. If not, i shall change JPEGIO_JPEG_MESSAGES to 0

Edit: some parts in WriteSlice could be updated too, but probably better in separate PR, this PR is already big.

issakomi · 2021-12-14T09:04:46Z

itkPyBufferMemoryLeakTest with ITK.macOS.Python fails again. It happens with many PRs. Not related.

S. InsightSoftwareConsortium#2928

dzenanz

LGTM, but since I opened the PR I cannot formally approve it.

hjmjohnson

Approving based on @dzenanz LGTM statement :).

github-actions Bot added area:IO Issues affecting the IO module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances labels Dec 9, 2021

dzenanz commented Dec 9, 2021

View reviewed changes

Comment thread Modules/IO/JPEG/src/itkJPEGImageIO.cxx

issakomi force-pushed the jpeg branch 2 times, most recently from ad226df to 7e80aae Compare December 9, 2021 17:12

issakomi force-pushed the jpeg branch from 7e80aae to 31e20ed Compare December 9, 2021 19:22

github-actions Bot added type:Data Changes to testing data type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct labels Dec 11, 2021

issakomi force-pushed the jpeg branch 2 times, most recently from 2889f5c to eb6bda6 Compare December 11, 2021 13:00

issakomi reviewed Dec 11, 2021

View reviewed changes

Comment thread Modules/IO/JPEG/include/itkJPEGImageIO.h

issakomi force-pushed the jpeg branch from eb6bda6 to 8c90a9d Compare December 12, 2021 08:08

issakomi approved these changes Dec 12, 2021

View reviewed changes

issakomi mentioned this pull request Dec 12, 2021

BUG: PNG IO fails on corrupt file #2942

Merged

issakomi reviewed Dec 12, 2021

View reviewed changes

issakomi force-pushed the jpeg branch from 8c90a9d to 19247b2 Compare December 12, 2021 13:09

jhlegarreta reviewed Dec 12, 2021

View reviewed changes

Comment thread Modules/IO/JPEG/src/itkJPEGImageIO.cxx

issakomi force-pushed the jpeg branch from 19247b2 to 3c76be7 Compare December 14, 2021 05:15

BUG: fixed undefined behaviour with corrupted JPEG file

028b6db

S. InsightSoftwareConsortium#2928

issakomi force-pushed the jpeg branch from 3c76be7 to 028b6db Compare December 14, 2021 13:27

dzenanz commented Dec 14, 2021

View reviewed changes

hjmjohnson approved these changes Dec 14, 2021

View reviewed changes

hjmjohnson merged commit d43311d into InsightSoftwareConsortium:master Dec 14, 2021

issakomi deleted the jpeg branch December 15, 2021 13:58

This was referenced Dec 17, 2021

BUG: updated JPEG IO #2975

Merged

ENH: JPEG IO: test another damaged file #2978

Closed

jhlegarreta mentioned this pull request Dec 23, 2021

ENH: JPEG IO test another damaged file #2981

Merged

blowekamp mentioned this pull request Apr 22, 2022

SimpleITK 2.1.1.2 causes a segfault when reading a truncated JPEG file from scikit-image SimpleITK/SimpleITK#1631

Closed

Uh oh!

Conversation

dzenanz commented Dec 9, 2021

Uh oh!

dzenanz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jhlegarreta commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

issakomi commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dzenanz commented Dec 9, 2021

Uh oh!

jhlegarreta commented Dec 11, 2021

Uh oh!

jhlegarreta commented Dec 11, 2021

Uh oh!

issakomi commented Dec 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

issakomi commented Dec 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hconk commented Dec 11, 2021

Uh oh!

Hconk commented Dec 11, 2021

Uh oh!

Uh oh!

issakomi Dec 12, 2021

Choose a reason for hiding this comment

Uh oh!

jhlegarreta Dec 12, 2021

Choose a reason for hiding this comment

Uh oh!

issakomi Dec 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhlegarreta Dec 12, 2021

Choose a reason for hiding this comment

Uh oh!

jhlegarreta Dec 12, 2021

Choose a reason for hiding this comment

Uh oh!

issakomi Dec 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

issakomi Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dzenanz commented Dec 13, 2021

Uh oh!

issakomi commented Dec 14, 2021

Uh oh!

issakomi commented Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

issakomi commented Dec 14, 2021

Uh oh!

dzenanz left a comment

Choose a reason for hiding this comment

Uh oh!

hjmjohnson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jhlegarreta commented Dec 9, 2021 •

edited

Loading

issakomi commented Dec 9, 2021 •

edited

Loading

issakomi commented Dec 11, 2021 •

edited

Loading

issakomi commented Dec 11, 2021 •

edited

Loading

issakomi Dec 12, 2021 •

edited

Loading

issakomi Dec 12, 2021 •

edited

Loading

issakomi Dec 13, 2021 •

edited

Loading

issakomi commented Dec 14, 2021 •

edited

Loading