long->sal_Int32 in tools/gen.hxx
sbergman at redhat.com
Thu Apr 5 07:53:29 UTC 2018
"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
6 <https://gerrit.libreoffice.org/52427> "Avoid
Other fallout from that "long->sal_Int32 in tools/gen.hxx" commit are
"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 &)()
> 191 Size aSize(rParam.nWidth, rParam.nHeight);
> 192 bool bGray = rParam.bGray;
> 194 mpBitmap.reset(new Bitmap());
>>>> 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();
> 198 if (nSize > SAL_MAX_INT32 / (bGray?1:3))
> 199 return false;
> 201 if( bGray )
"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.)
More information about the LibreOffice