Undefined behavior in jpeg.c

Post any defects you find in the released or beta versions of the ImageMagick software here. Include the ImageMagick version, OS, and any command-line required to reproduce the problem. Got a patch for a bug? Post it here.
Post Reply
peppe
Posts: 3
Joined: 2020-01-11T03:25:56-07:00
Authentication code: 1152

Undefined behavior in jpeg.c

Post by peppe »

Hi,

During some unrelated code analysis in Qt I've spotted that the setjmp / longjmp pairs in jpeg.c are triggering undefind behavior.

The analysis is as follows: in this code

https://github.com/ImageMagick/ImageMag ... eg.c#L2337

we have a variable (jpeg_info) which declared local to the function where the setjmp is called. This variable is written into after the setjmp (line 2344) and read again after the longjmp (line 2339). Since the object nor any of its subobjects is declared volatile, its status after the longjmp is indeterminate, and the read causes undefined behavior (cf. man 3 longjmp and paragraph §7.13.2.1.3 of www.open-std.org/jtc1/sc22/wg14/www/docs/n2346.pdf ).

Unfortunately one cannot simply mark jpeg_info as volatile -- the libjpeg's API don't want pointer-to-volatile-whatever (and casting volatile away and then touching the resulting object is undefined behavior again).

A very simple workaround is to declare jpeg_info (as well as any other non-volatile object touched after the longjmp) in a helper function, and pass them as a parameters to the function that does the actual work.

This is what happened in Qt here https://codereview.qt-project.org/c/qt/ ... er.cpp#729 as well as in libjpeg-turbo's own example code, which is likely the "root cause" of this similar code present everywhere https://github.com/libjpeg-turbo/libjpe ... 4d3d76b03e .

Thanks for reading.

User avatar
magick
Site Admin
Posts: 11216
Joined: 2003-05-31T11:32:55-07:00

Re: Undefined behavior in jpeg.c

Post by magick »

Thanks for the problem report. We can reproduce it and will have a patch to fix it in GIT master branch @ https://github.com/ImageMagick/ImageMagick later today. The patch will be available in the beta releases of ImageMagick @ https://www.imagemagick.org/download/beta/ by sometime tomorrow.

peppe
Posts: 3
Joined: 2020-01-11T03:25:56-07:00
Authentication code: 1152

Re: Undefined behavior in jpeg.c

Post by peppe »

Thank you very much for your quick response! Just a nag, the code now has "_ReadJPEGImage" "_WriteJPEGImage" as functions, and those names are not allowed in "user code" (that is, code not coming from the compiler/implementation) in C: "All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use, except those identifiers which are lexically identical to keywords." (N2346 §7.1.3.1). I really don't know if however that's an accepted style inside this project.

User avatar
magick
Site Admin
Posts: 11216
Joined: 2003-05-31T11:32:55-07:00

Re: Undefined behavior in jpeg.c

Post by magick »

Good catch. Will update to use ReadJPEGImage_() instead.

peppe
Posts: 3
Joined: 2020-01-11T03:25:56-07:00
Authentication code: 1152

Re: Undefined behavior in jpeg.c

Post by peppe »

Thank you again. I think it looks good now!

Post Reply