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()); 195CID 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?