New loplugin:unsignedcompare

"Find explicit casts from signed to unsigned integer in comparison against
unsigned integer, where the cast is presumably used to avoid warnings about
signed vs. unsigned comparisons, and could thus be replaced with
o3tl::make_unsigned for clairty." (compilerplugins/clang/unsignedcompare.cxx)

o3tl::make_unsigned requires its argument to be non-negative, and there is a
chance that some original code like

static_cast<sal_uInt32>(n) >= c

used the explicit cast to actually force a (potentially negative) value of
sal_Int32 to be interpreted as an unsigned sal_uInt32, rather than using the
cast to avoid a false "signed vs. unsigned comparison" warning in a case where
n is known to be non-negative.  It appears that restricting this plugin to non-
equality comparisons (<, >, <=, >=) and excluding equality comparisons (==, !=)
is a useful heuristic to avoid such false positives.  The only remainging false
positive I found was 0288c8ffecff4956a52b9147d441979941e8b87f "Rephrase cast
from sal_Int32 to sal_uInt32".

But which of course does not mean that there were no further false positivies
that I missed.  So this commit may accidentally introduce some false hits of the
assert in o3tl::make_unsigned.  At least, it passed a full (Linux ASan+UBSan
--enable-dbgutil) `make check && make screenshot`.

It is by design that o3tl::make_unsigned only accepts signed integer parameter
types (and is not defined as a nop for unsigned ones), to avoid unnecessary uses
which would in general be suspicious.  But the STATIC_ARRAY_SELECT macro in
include/oox/helper/helper.hxx is used with both signed and unsigned types, so
needs a little oox::detail::make_unsigned helper function for now.  (The
ultimate fix being to get rid of the macro in the first place.)

Change-Id: Ia4adc9f44c70ad1dfd608784cac39ee922c32175
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/87556
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>

8 files changed, 26 insertions, 18 deletions

diff --git a/filter/source/graphicfilter/icgm/cgm.cxx b/filter/source/graphicfilter/icgm/cgm.cxx
index 09abcac8d136..9a13e5ebb99e 100644
--- a/filter/source/graphicfilter/icgm/cgm.cxx
+++ b/filter/source/graphicfilter/icgm/cgm.cxx
@@ -18,7 +18,7 @@
  */
 
 #include <com/sun/star/task/XStatusIndicator.hpp>
-
+#include <o3tl/safeint.hxx>
 #include <osl/endian.h>
 #include <tools/stream.hxx>
 #include "bitmap.hxx"
@@ -95,7 +95,7 @@ sal_uInt8 CGM::ImplGetByte( sal_uInt32 nSource, sal_uInt32 nPrecision )
 sal_Int32 CGM::ImplGetI( sal_uInt32 nPrecision )
 {
     sal_uInt8* pSource = mpSource + mnParaSize;
-    if (pSource > mpEndValidSource || static_cast<sal_uIntPtr>(mpEndValidSource - pSource) < nPrecision)
+    if (pSource > mpEndValidSource || o3tl::make_unsigned(mpEndValidSource - pSource) < nPrecision)
         throw css::uno::Exception("attempt to read past end of input", nullptr);
     mnParaSize += nPrecision;
     switch( nPrecision )
@@ -127,7 +127,7 @@ sal_Int32 CGM::ImplGetI( sal_uInt32 nPrecision )
 sal_uInt32 CGM::ImplGetUI( sal_uInt32 nPrecision )
 {
     sal_uInt8* pSource = mpSource + mnParaSize;
-    if (pSource > mpEndValidSource || static_cast<sal_uIntPtr>(mpEndValidSource - pSource) < nPrecision)
+    if (pSource > mpEndValidSource || o3tl::make_unsigned(mpEndValidSource - pSource) < nPrecision)
         throw css::uno::Exception("attempt to read past end of input", nullptr);
     mnParaSize += nPrecision;
     switch( nPrecision )
@@ -182,7 +182,7 @@ double CGM::ImplGetFloat( RealPrecision eRealPrecision, sal_uInt32 nRealSize )
     const bool bCompatible = false;
 #endif
 
-    if (static_cast<sal_uIntPtr>(mpEndValidSource - (mpSource + mnParaSize)) < nRealSize)
+    if (o3tl::make_unsigned(mpEndValidSource - (mpSource + mnParaSize)) < nRealSize)
         throw css::uno::Exception("attempt to read past end of input", nullptr);
 
     if ( bCompatible )
diff --git a/filter/source/graphicfilter/icgm/class1.cxx b/filter/source/graphicfilter/icgm/class1.cxx
index 30e28dd820cc..ec8cd9246cdb 100644
--- a/filter/source/graphicfilter/icgm/class1.cxx
+++ b/filter/source/graphicfilter/icgm/class1.cxx
@@ -188,7 +188,7 @@ void CGM::ImplDoClass1()
                 ImplGetUI16(); // skip CharSetType
                 sal_uInt32 nSize = ImplGetUI(1);
 
-                if (static_cast<sal_uIntPtr>(mpEndValidSource - (mpSource + mnParaSize)) < nSize)
+                if (o3tl::make_unsigned(mpEndValidSource - (mpSource + mnParaSize)) < nSize)
                     throw css::uno::Exception("attempt to read past end of input", nullptr);
 
                 pElement->aFontList.InsertCharSet( mpSource + mnParaSize, nSize );
diff --git a/filter/source/graphicfilter/icgm/class7.cxx b/filter/source/graphicfilter/icgm/class7.cxx
index 5de4680e08f6..1b51ce98e71c 100644
--- a/filter/source/graphicfilter/icgm/class7.cxx
+++ b/filter/source/graphicfilter/icgm/class7.cxx
@@ -17,6 +17,9 @@
  *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
  */
 
+#include <sal/config.h>
+
+#include <o3tl/safeint.hxx>
 
 #include "cgm.hxx"
 #include "chart.hxx"
@@ -75,7 +78,7 @@ void CGM::ImplDoClass7()
                     case 0x262 : /*AppData - ENDGROUP */break;
                     case 0x264 : /*AppData - DATANODE*/
                     {
-                        if (static_cast<size_t>(mpEndValidSource - pAppData) < sizeof(DataNode))
+                        if (o3tl::make_unsigned(mpEndValidSource - pAppData) < sizeof(DataNode))
                             throw css::uno::Exception("attempt to read past end of input", nullptr);
 
                         mpChart->mDataNode[ 0 ] = *reinterpret_cast<DataNode*>( pAppData );
diff --git a/filter/source/graphicfilter/idxf/dxfentrd.cxx b/filter/source/graphicfilter/idxf/dxfentrd.cxx
index dfc14ba0edcb..883dcc991818 100644
--- a/filter/source/graphicfilter/idxf/dxfentrd.cxx
+++ b/filter/source/graphicfilter/idxf/dxfentrd.cxx
@@ -17,6 +17,9 @@
  *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
  */
 
+#include <sal/config.h>
+
+#include <o3tl/safeint.hxx>
 
 #include "dxfentrd.hxx"
 
@@ -417,7 +420,7 @@ void DXFLWPolyLineEntity::EvaluateGroup( DXFGroupReader & rDGR )
         {
             nCount = rDGR.GetI();
             // limit alloc to max reasonable size based on remaining data in stream
-            if (nCount > 0 && static_cast<sal_uInt32>(nCount) <= rDGR.remainingSize())
+            if (nCount > 0 && o3tl::make_unsigned(nCount) <= rDGR.remainingSize())
                 aP.reserve(nCount);
             else
                 nCount = 0;
@@ -584,7 +587,7 @@ bool DXFBoundaryPathData::EvaluateGroup( DXFGroupReader & rDGR )
             {
                 nPointCount = rDGR.GetI();
                 // limit alloc to max reasonable size based on remaining data in stream
-                if (nPointCount > 0 && static_cast<sal_uInt32>(nPointCount) <= rDGR.remainingSize())
+                if (nPointCount > 0 && o3tl::make_unsigned(nPointCount) <= rDGR.remainingSize())
                     aP.reserve(nPointCount);
                 else
                     nPointCount = 0;
@@ -672,7 +675,7 @@ void DXFHatchEntity::EvaluateGroup( DXFGroupReader & rDGR )
             bIsInBoundaryPathContext = true;
             nBoundaryPathCount = rDGR.GetI();
             // limit alloc to max reasonable size based on remaining data in stream
-            if (nBoundaryPathCount > 0 && static_cast<sal_uInt32>(nBoundaryPathCount) <= rDGR.remainingSize())
+            if (nBoundaryPathCount > 0 && o3tl::make_unsigned(nBoundaryPathCount) <= rDGR.remainingSize())
                 pBoundaryPathData.reset( new DXFBoundaryPathData[ nBoundaryPathCount ] );
             else
                 nBoundaryPathCount = 0;
diff --git a/filter/source/graphicfilter/ieps/ieps.cxx b/filter/source/graphicfilter/ieps/ieps.cxx
index 7cf34b6493aa..1aaed97295b6 100644
--- a/filter/source/graphicfilter/ieps/ieps.cxx
+++ b/filter/source/graphicfilter/ieps/ieps.cxx
@@ -493,7 +493,7 @@ static void MakePreview(sal_uInt8* pBuf, sal_uInt32 nBytesRead,
             --nRemainingBytes;
         }
         nLen = ImplGetLen(pDest, std::min<sal_uInt32>(nRemainingBytes, 32));
-        if (static_cast<sal_uInt32>(nLen) < nRemainingBytes)
+        if (o3tl::make_unsigned(nLen) < nRemainingBytes)
         {
             sal_uInt8 aOldValue(pDest[ nLen ]); pDest[ nLen ] = 0;
             if ( strcmp( reinterpret_cast<char*>(pDest), "none" ) != 0 )
@@ -516,7 +516,7 @@ static void MakePreview(sal_uInt8* pBuf, sal_uInt32 nBytesRead,
             --nRemainingBytes;
         }
         nLen = ImplGetLen(pDest, std::min<sal_uInt32>(nRemainingBytes, 32));
-        if (static_cast<sal_uInt32>(nLen) < nRemainingBytes)
+        if (o3tl::make_unsigned(nLen) < nRemainingBytes)
         {
             sal_uInt8 aOldValue(pDest[nLen]); pDest[nLen] = 0;
             const char* pStr = reinterpret_cast<char*>(pDest);
@@ -536,7 +536,7 @@ static void MakePreview(sal_uInt8* pBuf, sal_uInt32 nBytesRead,
             --nRemainingBytes;
         }
         nLen = ImplGetLen(pDest, std::min<sal_uInt32>(nRemainingBytes, 32));
-        if (static_cast<sal_uInt32>(nLen) < nRemainingBytes)
+        if (o3tl::make_unsigned(nLen) < nRemainingBytes)
         {
             sal_uInt8 aOldValue(pDest[ nLen ]); pDest[ nLen ] = 0;
             if ( strcmp( reinterpret_cast<char*>(pDest), "none" ) != 0 )
diff --git a/filter/source/msfilter/msdffimp.cxx b/filter/source/msfilter/msdffimp.cxx
index 753b3b11a69e..800a675dc54b 100644
--- a/filter/source/msfilter/msdffimp.cxx
+++ b/filter/source/msfilter/msdffimp.cxx
@@ -25,6 +25,7 @@
 #include <vector>
 
 #include <o3tl/any.hxx>
+#include <o3tl/safeint.hxx>
 #include <osl/file.hxx>
 #include <tools/solar.h>
 #include <sal/log.hxx>
@@ -719,7 +720,7 @@ void SvxMSDffManager::SolveSolver( const SvxMSDffSolverContainer& rSolver )
                                     {
                                         css::uno::Sequence< css::drawing::EnhancedCustomShapeParameterPair > aCoordinates;
                                         *pAny >>= aCoordinates;
-                                        if ( nPt < static_cast<sal_uInt32>(aCoordinates.getLength()) )
+                                        if ( nPt < o3tl::make_unsigned(aCoordinates.getLength()) )
                                         {
                                             nId = 4;
                                             css::drawing::EnhancedCustomShapeParameterPair& rPara = aCoordinates[ nPt ];
@@ -4217,7 +4218,7 @@ SdrObject* SvxMSDffManager::ImportGroup( const DffRecordHeader& rHd, SvStream& r
             }
         }
     }
-    if (size_t(nCalledByGroup) < maPendingGroupData.size())
+    if (o3tl::make_unsigned(nCalledByGroup) < maPendingGroupData.size())
     {
         // finalization for this group is pending, do it now
         pRet = FinalizeObj(maPendingGroupData.back().first, pRet);
@@ -4961,7 +4962,7 @@ SdrObject* SvxMSDffManager::ImportShape( const DffRecordHeader& rHd, SvStream& r
     // If this shape opens a new group, push back its object data because
     // finalization will be called when nested objects have been imported;
     // otherwise, just finalize here
-    if (size_t(nCalledByGroup) > maPendingGroupData.size())
+    if (o3tl::make_unsigned(nCalledByGroup) > maPendingGroupData.size())
     {
         auto xHdClone = std::make_shared<DffRecordHeader>(aObjData.rSpHd);
         maPendingGroupData.emplace_back(DffObjData(xHdClone, aObjData), xHdClone );
diff --git a/filter/source/msfilter/mstoolbar.cxx b/filter/source/msfilter/mstoolbar.cxx
index 7f0e7d70b9f6..f44cd2bbb3ca 100644
--- a/filter/source/msfilter/mstoolbar.cxx
+++ b/filter/source/msfilter/mstoolbar.cxx
@@ -7,6 +7,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 #include <filter/msfilter/mstoolbar.hxx>
+#include <o3tl/safeint.hxx>
 #include <sal/log.hxx>
 #include <com/sun/star/beans/XPropertySet.hpp>
 #include <com/sun/star/container/XIndexContainer.hpp>
@@ -676,7 +677,7 @@ bool TBCCDData::Read( SvStream &rS)
     if (cwstrItems > 0)
     {
         //each WString is at least one byte
-        if (rS.remainingSize() < static_cast<size_t>(cwstrItems))
+        if (rS.remainingSize() < o3tl::make_unsigned(cwstrItems))
             return false;
         for( sal_Int32 index=0; index < cwstrItems; ++index )
         {
diff --git a/filter/source/msfilter/svdfppt.cxx b/filter/source/msfilter/svdfppt.cxx
index 83856b5d5499..1cae88f4870f 100644
--- a/filter/source/msfilter/svdfppt.cxx
+++ b/filter/source/msfilter/svdfppt.cxx
@@ -1217,7 +1217,7 @@ SdrObject* SdrEscherImport::ProcessObj( SvStream& rSt, DffObjData& rObjData, Svx
                     rSt.ReadInt16( nRowCount ).ReadInt16( i ).ReadInt16( i );
                     const size_t nMinRecordSize = 4;
                     const size_t nMaxRecords = rSt.remainingSize() / nMinRecordSize;
-                    if (nRowCount > 0 && static_cast<size_t>(nRowCount) > nMaxRecords)
+                    if (nRowCount > 0 && o3tl::make_unsigned(nRowCount) > nMaxRecords)
                     {
                         SAL_WARN("filter.ms", "Parsing error: " << nMaxRecords <<
                                  " max possible entries, but " << nRowCount << " claimed, truncating");
@@ -5366,7 +5366,7 @@ void PPTStyleTextPropReader::Init( SvStream& rIn, const DffRecordHeader& rTextHe
                 }
                 else
                 {
-                    if (nCharReadCnt > static_cast<sal_uInt32>(aString.getLength()))
+                    if (nCharReadCnt > o3tl::make_unsigned(aString.getLength()))
                         aCharPropSet.maString = OUString();
                     else
                     {