Date: prev next · Thread: first prev next last
2018 Archives by date, by thread · List index


<https://cgit.freedesktop.org/libreoffice/core/commit/?id=8bc951daf79decbd8a599a409c6d33c5456710e0> "long->sal_Int32 in tools/gen.hxx" changed tools/gen.hxx classes like Point and Rect from being based on long members to being based on sal_Int32 members. It was probably a good idea to make the type of those members platform-independent, so that behavior will not unnecessarily differ among platforms. Relevant choices for platform-independent type were apparently sal_Int32 and sal_Int64. Choosing sal_Int32 seems reasonable, given that this choice obviously worked well already for all platforms where long is 32 bit (which includes all variants of Windows, x86 and x86-64). Or did it?

For Linux x86-64 (where long is 64 bit) UBSan builds (which flag signed integer overflow as undefined behavior), that commit is a disaster. (And it is unfortunate that that commit wasn't run past such a build before hitting master.)

I have six pending Gerrit patches that would make `make check screenshot` succeed again for my Linux x86-64 UBSan build. I don't really like none of those six; they appear to be getting lost in fixing up symptoms rather than addressing root causes:

1 <https://gerrit.libreoffice.org/#/c/52335/> "Pull FAR_AWAY a little closer to avoid -fsanitize=signed-integer-overflow" 2 <https://gerrit.libreoffice.org/#/c/52339/> "Make sure rectangle doesn't get too large" 3 <https://gerrit.libreoffice.org/52424> "Cap svg:width/height values to avoid later -fsanitize=signed-integer-overflow" 4 <https://gerrit.libreoffice.org/52425> "Reduce ImpEditEngine::aPaperSize to avoid -fsanitize=signed-integer-overflow" 5 <https://gerrit.libreoffice.org/52426> " Reduce EMPTY_PAPER_SIZE to avoid -fsanitize=signed-integer-overflow" 6 <https://gerrit.libreoffice.org/52427> "Avoid -fsanitize=signed-integer-overflow"

Other fallout from that "long->sal_Int32 in tools/gen.hxx" commit are <https://cgit.freedesktop.org/libreoffice/core/commit/?id=9762c9ab98a2b9ea86186a6da7b77031f1416bed> "ofz: lots of integer overflow noise" and Coverity Scan's recent finding of

*** CID 1433796:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
/vcl/source/filter/jpeg/JpegReader.cxx: 196 in JPEGReader::CreateBitmap(const JPEGCreateBitmapParam 
&)()
190 191 Size aSize(rParam.nWidth, rParam.nHeight);
192         bool bGray = rParam.bGray;
193 194 mpBitmap.reset(new Bitmap()); 195
    CID 1433796:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
    Potentially overflowing expression "aSize.Width() * aSize.Height()" with type "int" (32 bits, signed) is 
evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "sal_uInt64" (64 bits, 
unsigned).
196         sal_uInt64 nSize = aSize.Width() * aSize.Height();
197 198 if (nSize > SAL_MAX_INT32 / (bGray?1:3))
199             return false;
200 201 if( bGray )

(<https://lists.freedesktop.org/archives/libreoffice/2018-April/079953.html> "New Defects reported by Coverity Scan for LibreOffice").

I propose that we revisit the "long->sal_Int32 in tools/gen.hxx" commit and change to sal_Int64 instead:

* At least some of my Gerrit patches involve overflows that just wouldn't happen with sal_Int64, either because they operate on hard-coded values close to SAL_MAX_INT32 (1, 4, 5) or because they operate on "tainted" 32-bit values read from document files (2).

* Other cases could probably overflow even for sal_Int64, so need fixing. While those cases are probably easier to find for sal_Int32 (as they'll hit more easily), I think that doesn't outweigh the disadvantages we've moved ourself into with that commit. It is unreasonable to assume that we'll properly fix all those places of potential overflow anytime soon. But they'd keep plaguing us with UBSan and ofz and Coverity Scan and..., and we'd keep addressing them with more-or-less thought-through ad-hoc fixes for the symptoms rather than the root causes, increasing the mess factor of the code base even further. (And I assume it is unlikely that those potential overflows are sources of exploitable security issues, given all this is about geometry classes like Point and Rect. What would typically happen, I guess, when such an overflow hits is that layout gets distorted.)

Thoughts?

Context


Privacy Policy | Impressum (Legal Info) | Copyright information: Unless otherwise specified, all text and images on this website are licensed under the Creative Commons Attribution-Share Alike 3.0 License. This does not include the source code of LibreOffice, which is licensed under the Mozilla Public License (MPLv2). "LibreOffice" and "The Document Foundation" are registered trademarks of their corresponding registered owners or are in actual use as trademarks in one or more countries. Their respective logos and icons are also subject to international copyright laws. Use thereof is explained in our trademark policy.