From 58c4fa1061d07d35805bbb5cc8af7b36129d4509 Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Thu, 10 Oct 2019 16:07:55 +0200 Subject: JPEG image handler: drop a use of "volatile" The reading code protects a local variable with volatile. In this case the only possible reason to apply volatile seems to be protecting the variable across the subsequent setjmp/longjmp. However, the variable is never accessed after the longjmp (the function returns). So, drop the volatile. Change-Id: Ibecb11a9edcc6027b2fd52b555287ad53375a5d0 Reviewed-by: Allan Sandfeld Jensen Reviewed-by: Edward Welbourne --- src/plugins/imageformats/jpeg/qjpeghandler.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'src/plugins') diff --git a/src/plugins/imageformats/jpeg/qjpeghandler.cpp b/src/plugins/imageformats/jpeg/qjpeghandler.cpp index 1f1675e490..a09e6dfd5a 100644 --- a/src/plugins/imageformats/jpeg/qjpeghandler.cpp +++ b/src/plugins/imageformats/jpeg/qjpeghandler.cpp @@ -248,13 +248,12 @@ static bool ensureValidImage(QImage *dest, struct jpeg_decompress_struct *info, static bool read_jpeg_image(QImage *outImage, QSize scaledSize, QRect scaledClipRect, - QRect clipRect, volatile int inQuality, + QRect clipRect, int quality, Rgb888ToRgb32Converter converter, j_decompress_ptr info, struct my_error_mgr* err ) { if (!setjmp(err->setjmp_buffer)) { // -1 means default quality. - int quality = inQuality; if (quality < 0) quality = 75; -- cgit v1.2.3 From 8aa3329a7186b087163f68ef880c8ffdd47d01bf Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Thu, 10 Oct 2019 12:20:05 +0200 Subject: JPEG image handler: remove undefined behavior from setjmp/longjmp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Edward Welbourne --- src/plugins/imageformats/jpeg/qjpeghandler.cpp | 34 ++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 5 deletions(-) (limited to 'src/plugins') 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 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; } -- cgit v1.2.3