[Libreoffice-commits] core.git: vcl/source

[Stephan Bergmann (via logerrit)]

 vcl/source/gdi/TypeSerializer.cxx |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

New commits:
commit cb310560a887ba08ea4234ea6cdd217151ca0728
Author:     Stephan Bergmann <sbergman at redhat.com>
AuthorDate: Thu Oct 22 11:05:07 2020 +0200
Commit:     Stephan Bergmann <sbergman at redhat.com>
CommitDate: Thu Oct 22 14:03:53 2020 +0200

    Guard against bad stream in TypeSerializer::readGradient
    
    ...by defaulting to zero any values that fail to get read, in line with how
    other TypeSerializer::read* (vcl/source/gdi/TypeSerializer.cxx) and underlying
    GenericTypeSerializer::read* (tools/source/stream/GenericTypeSerializer.cxx)
    functions handle bad streams (though TypeSerializer::readGraphic does check
    mrStream's state, as a notable exception).
    
    I observed such failed reads with `VALGRIND=memcheck make
    CppunitTest_vcl_filters_test CPPUNIT_TEST_NAME=VclFiltersTest::testCVEs`:
    
    > Conditional jump or move depends on uninitialised value(s)
    >    at 0x170EA438: TypeSerializer::readGradient(Gradient&) (/vcl/source/gdi/TypeSerializer.cxx:61)
    >    by 0x16F00CEE: MetaFloatTransparentAction::Read(SvStream&, ImplMetaReadData*) (/vcl/source/gdi/metaact.cxx:3020)
    >    by 0x16EF1704: MetaAction::ReadMetaAction(SvStream&, ImplMetaReadData*) (/vcl/source/gdi/metaact.cxx:269)
    >    by 0x16E99C03: ReadGDIMetaFile(SvStream&, GDIMetaFile&, ImplMetaReadData*) (/vcl/source/gdi/gdimtf.cxx:2693)
    >    by 0x16F00C91: MetaFloatTransparentAction::Read(SvStream&, ImplMetaReadData*) (/vcl/source/gdi/metaact.cxx:3016)
    >    by 0x16EF1704: MetaAction::ReadMetaAction(SvStream&, ImplMetaReadData*) (/vcl/source/gdi/metaact.cxx:269)
    >    by 0x16E99C03: ReadGDIMetaFile(SvStream&, GDIMetaFile&, ImplMetaReadData*) (/vcl/source/gdi/gdimtf.cxx:2693)
    >    by 0x16F00C91: MetaFloatTransparentAction::Read(SvStream&, ImplMetaReadData*) (/vcl/source/gdi/metaact.cxx:3016)
    >    by 0x16EF1704: MetaAction::ReadMetaAction(SvStream&, ImplMetaReadData*) (/vcl/source/gdi/metaact.cxx:269)
    >    by 0x16E99C03: ReadGDIMetaFile(SvStream&, GDIMetaFile&, ImplMetaReadData*) (/vcl/source/gdi/gdimtf.cxx:2693)
    >  Uninitialised value was created by a stack allocation
    >    at 0x170EA274: TypeSerializer::readGradient(Gradient&) (/vcl/source/gdi/TypeSerializer.cxx:33)
    
    (The uninitialized
    
      sal_uInt16 nAngle
    
    had started to cause issues since 0fb58a1ff168ae122e9c8993a3136658e3b0e3f0 "new
    tools::Degree10 strong typedef", when e.g.
    <https://ci.libreoffice.org/job/lo_tb_master_linux_dbg/30821/> failed with
    
    > cppunittester: /home/tdf/lode/jenkins/workspace/lo_tb_master_linux_dbg/include/o3tl/strong_int.hxx:94: constexpr o3tl::strong_int<UNDERLYING_TYPE, PHANTOM_TYPE>::strong_int(T, typename std::enable_if<std::is_integral<T>::value, int>::type) [with T = short unsigned int; UNDERLYING_TYPE = short int; PHANTOM_TYPE = Degree10Tag; typename std::enable_if<std::is_integral<T>::value, int>::type = int]: Assertion `detail::isInRange<UNDERLYING_TYPE>(value) && "out of range"' failed.
    
    apparently because nAngle happened to contain a value >
    std::limits<sal_Int32>::max(), so converting it to a tools::Degree10 based on
    sal_Int16 triggered the assert.  0e8e352cc78af9b8cee27a77b0ac8e2e8f98f8cc "clamp
    angle to valid value" tried to address the symptoms, but failed to identify the
    root cause which is now addressed in this commit.  I'm not sure whether that
    clamping of nAngle from 0e8e352cc78af9b8cee27a77b0ac8e2e8f98f8cc is useful in
    itself, or whether nAngle should rather be of type sal_Int16 (or converted to
    such prior to being converted to Degree10).  But that question of what input
    values (if they are actually read in from a non-broken stream) are invalid and
    need treatment is somewhat orthogonal to this fix, so I leave that as it
    currently is for now.)
    
    Change-Id: Iae74bad9e2543bf7ac8712adbf360d5e1076bdf7
    Reviewed-on: https://gerrit.libreoffice.org/c/core/+/104650
    Tested-by: Jenkins
    Reviewed-by: Stephan Bergmann <sbergman at redhat.com>

diff --git a/vcl/source/gdi/TypeSerializer.cxx b/vcl/source/gdi/TypeSerializer.cxx
index bbfdbf430160..c86498485d4f 100644
--- a/vcl/source/gdi/TypeSerializer.cxx
+++ b/vcl/source/gdi/TypeSerializer.cxx
@@ -33,16 +33,16 @@ void TypeSerializer::readGradient(Gradient& rGradient)
 {
     VersionCompat aCompat(mrStream, StreamMode::READ);
 
-    sal_uInt16 nStyle;
+    sal_uInt16 nStyle = 0;
     Color aStartColor;
     Color aEndColor;
-    sal_uInt16 nAngle;
-    sal_uInt16 nBorder;
-    sal_uInt16 nOffsetX;
-    sal_uInt16 nOffsetY;
-    sal_uInt16 nIntensityStart;
-    sal_uInt16 nIntensityEnd;
-    sal_uInt16 nStepCount;
+    sal_uInt16 nAngle = 0;
+    sal_uInt16 nBorder = 0;
+    sal_uInt16 nOffsetX = 0;
+    sal_uInt16 nOffsetY = 0;
+    sal_uInt16 nIntensityStart = 0;
+    sal_uInt16 nIntensityEnd = 0;
+    sal_uInt16 nStepCount = 0;
 
     mrStream.ReadUInt16(nStyle);
     readColor(aStartColor);