summaryrefslogtreecommitdiffstats
path: root/src/plugins
diff options
context:
space:
mode:
authorGiuseppe D'Angelo <giuseppe.dangelo@kdab.com>2019-10-10 12:20:05 +0200
committerGiuseppe D'Angelo <giuseppe.dangelo@kdab.com>2019-10-11 19:57:36 +0200
commit8aa3329a7186b087163f68ef880c8ffdd47d01bf (patch)
tree86df0bb6d6442f55344d205945c10266961f8d15 /src/plugins
parent58c4fa1061d07d35805bbb5cc8af7b36129d4509 (diff)
JPEG image handler: remove undefined behavior from setjmp/longjmp
The JPEG writing code features a setjmp/longjmp pair to deal with error handling. In doing so, it creates UB by touching local objects after the setjmp and then after the corresponding longjmp. The rules on what we can do are quite strict: objects that are 1) local to the function calling setjmp; 2) not qualified with volatile; 3) written into after the setjmp; have indeterminate state after the corresponding longjmp call (man 3 longjmp, C 2x draft N2346 §7.13.2.1.2). Not making any assumptions on any compiler used: let's just say that using them in any way is UB. Luckily, no compiler exploits this (yet), and the code works just fine. But we know the drill -- never play this game against compilers, because you will lose. So: we have a couple of those objects around in the writing routine (cinfo, row_pointer), that violate the rules above. Unfortunately we can't simply mark them as volatile: libjpeg's API expects them not to be volatile. Casting volatileness away and then touching an object in any way is undefined behavior out of the bat (C 2x draft N2346 §6.7.3.7, C++ [dcl.type.cv]). Given the code needs to do 3), and we can't work around 2), then work around 1): define them to be non-local to the function doing the setjmp. Introduce a small helper that declares such objects and then calls the function doing the actual work, with the setjmp/longjmp. An overall alternative would be of course stop using setjmp/longjmp, but libjpeg's API doesn't really seem to allow this -- when the library calls user's error handler, that error handler is expected not to return to the library (so a longjmp or an exit/abort are mandatory). Side note: all the code using libjpeg I've researched to debug this has this very same strange issue: * GDK-pixbuf's [1] * ImageMagick's [2] * and even libjpeg's [3] and libjpeg-turbo's [4] own examples. [1] https://github.com/GNOME/gdk-pixbuf/blob/master/gdk-pixbuf/io-jpeg.c#L581 [2] https://github.com/ImageMagick/ImageMagick/blob/master/coders/jpeg.c#L2338 [3] https://www.ijg.org/ [4] https://github.com/libjpeg-turbo/libjpeg-turbo/blob/master/example.txt#L331 Change-Id: I34a810db468f73423478cd3ac71b888f4b11cb28 Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io> Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>
Diffstat (limited to 'src/plugins')
-rw-r--r--src/plugins/imageformats/jpeg/qjpeghandler.cpp34
1 files changed, 29 insertions, 5 deletions
diff --git a/src/plugins/imageformats/jpeg/qjpeghandler.cpp b/src/plugins/imageformats/jpeg/qjpeghandler.cpp
index a09e6dfd5a..dd01138722 100644
--- a/src/plugins/imageformats/jpeg/qjpeghandler.cpp
+++ b/src/plugins/imageformats/jpeg/qjpeghandler.cpp
@@ -528,7 +528,14 @@ static inline void write_icc_profile(const QImage &image, j_compress_ptr cinfo)
}
}
-static bool write_jpeg_image(const QImage &image, QIODevice *device, volatile int sourceQuality, const QString &description, bool optimize, bool progressive)
+static bool do_write_jpeg_image(struct jpeg_compress_struct &cinfo,
+ JSAMPROW *row_pointer,
+ const QImage &image,
+ QIODevice *device,
+ int sourceQuality,
+ const QString &description,
+ bool optimize,
+ bool progressive)
{
bool success = false;
const QVector<QRgb> cmap = image.colorTable();
@@ -536,10 +543,6 @@ static bool write_jpeg_image(const QImage &image, QIODevice *device, volatile in
if (image.format() == QImage::Format_Invalid || image.format() == QImage::Format_Alpha8)
return false;
- struct jpeg_compress_struct cinfo;
- JSAMPROW row_pointer[1];
- row_pointer[0] = 0;
-
struct my_jpeg_destination_mgr *iod_dest = new my_jpeg_destination_mgr(device);
struct my_error_mgr jerr;
@@ -712,6 +715,27 @@ static bool write_jpeg_image(const QImage &image, QIODevice *device, volatile in
}
delete iod_dest;
+ return success;
+}
+
+static bool write_jpeg_image(const QImage &image,
+ QIODevice *device,
+ int sourceQuality,
+ const QString &description,
+ bool optimize,
+ bool progressive)
+{
+ // protect these objects from the setjmp/longjmp pair inside
+ // do_write_jpeg_image (by making them non-local).
+ struct jpeg_compress_struct cinfo;
+ JSAMPROW row_pointer[1];
+ row_pointer[0] = 0;
+
+ const bool success = do_write_jpeg_image(cinfo, row_pointer,
+ image, device,
+ sourceQuality, description,
+ optimize, progressive);
+
delete [] row_pointer[0];
return success;
}