long->sal_Int32 in tools/gen.hxx

[Stephan Bergmann]

<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?