Re: [lmi] expand_html() msw64: 'SIZE_MAX <= index' always false

[Vadim Zeitlin]

On Sun, 24 Mar 2019 10:09:21 +0000 Greg Chicares <address@hidden> wrote:

GC> Vadim--is this the right fix for the problem below?
GC> 
GC> 
---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
GC> diff --git a/pdf_command_wx.cpp b/pdf_command_wx.cpp
GC> index c90f4938..3a40f278 100644
GC> --- a/pdf_command_wx.cpp
GC> +++ b/pdf_command_wx.cpp
GC> @@ -269,7 +269,7 @@ class html_interpolator
GC>                  }
GC>  
GC>              char* stop = nullptr;
GC> -            auto const index = std::strtoul(s.c_str() + open_pos + 1, 
&stop, 10);
GC> +            auto const index = std::strtoull(s.c_str() + open_pos + 1, 
&stop, 10);
GC>  
GC>              // Conversion must have stopped at the closing bracket 
character
GC>              // and also check for overflow (notice that index == SIZE_MAX
GC> 
--------->8-------->8-------->8-------->8-------->8-------->8-------->8-------
GC> 
GC> To reproduce:
GC> 
GC> x86_64-w64-mingw32-g++ -MMD -MP -MT pdf_command_wx.o -MF pdf_command_wx.d  
-c -I /opt/lmi/src/lmi -I /opt/lmi/src/lmi/tools/pete-2.1.1 -I 
/opt/lmi/x86_64-w64-mingw32/local/lib/wx/include/x86_64-w64-mingw32-msw-unicode-3.1
 -I /opt/lmi/x86_64-w64-mingw32/local/include/wx-3.1 -I 
/opt/lmi/third_party/include -I /opt/lmi/third_party/src -I 
/opt/lmi/x86_64-w64-mingw32/local/include -I 
/opt/lmi/x86_64-w64-mingw32/local/include/libxml2 -DLMI_WX_NEW_USE_SO  
-DLIBXML_USE_DLL -DSTRICT    -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL -D__WXMSW__ 
-DBOOST_NO_AUTO_PTR -DBOOST_STRICT_CONFIG -DBOOST_STATIC_ASSERT_HPP   
-frounding-math -std=c++17 -pedantic-errors -Werror -Wall -Walloc-zero -Walloca 
-Wcast-align -Wconversion -Wdangling-else -Wdeprecated-declarations 
-Wdisabled-optimization -Wdouble-promotion -Wduplicated-branches 
-Wduplicated-cond -Wextra -Wformat-nonliteral -Wformat-security 
-Wformat-signedness -Wformat-y2k -Wimport -Winit-self -Winvalid-pch 
-Wlogical-op -Wmissing-include-dirs -Wmultichar -Wnull-dereference -Wpacked 
-Wpointer-arith -Wredundant-decls -Wrestrict -Wshadow -Wsign-compare 
-Wstack-protector -Wswitch-enum -Wtrampolines -Wundef -Wunreachable-code 
-Wunused-macros -Wvector-operation-performance -Wwrite-strings -Wno-parentheses 
 -Wc++11-compat -Wc++14-compat -Wc++1z-compat -Wconditionally-supported 
-Wctor-dtor-privacy -Wdelete-non-virtual-dtor -Wdeprecated -Wnoexcept 
-Wnoexcept-type -Wnon-template-friend -Wnon-virtual-dtor -Woverloaded-virtual 
-Wpmf-conversions -Wregister -Wreorder -Wstrict-null-sentinel -Wsynth 
-Wuseless-cast  -Wcast-qual    -D'BOOST_STATIC_ASSERT(A)=static_assert((A))'   
-ggdb -O2    /opt/lmi/src/lmi/pdf_command_wx.cpp -opdf_command_wx.o
GC> 
GC> /opt/lmi/src/lmi/pdf_command_wx.cpp: In member function 'html::text 
{anonymous}::html_interpolator::expand_html(const string&) const':
GC> /opt/lmi/src/lmi/pdf_command_wx.cpp:278:63: error: comparison is always 
false due to limited range of data type [-Werror=type-limits]
GC>              if(stop != s.c_str() + s.length() - 1 || SIZE_MAX <= index)
GC>                                                                ^
GC> cc1plus: all warnings being treated as errors

 Sorry, I'm not sure why would we want to use long long here when the
indices are typically small integers, i.e. should fit in a short if not
char. The real problem seems to be a rather head-scratching use of SIZE_MAX
here, to be honest I have absolutely no recollection why did I decide to do
it (and out Git history doesn't contain the details of my commits in that
branch, so even if I had written a message explaining it back when I added
it, it's not available any longer...).

 Looking at the code anew it seems that we should really be using ULONG_MAX
instead, which wouldn't generate this warning (I've tested and confirmed
that this is indeed the case). I.e. I'd rather propose this patch instead:

---------------------------------- >8 --------------------------------------
diff --git a/pdf_command_wx.cpp b/pdf_command_wx.cpp
index 1493d2f90..75d5cc7f5 100644
--- a/pdf_command_wx.cpp
+++ b/pdf_command_wx.cpp
@@ -51,7 +51,7 @@
 #include <wx/html/m_templ.h>

 #include <array>
-#include <cstdint>                      // SIZE_MAX
+#include <cstdint>                      // ULONG_MAX
 #include <cstdlib>                      // strtoul()
 #include <exception>                    // uncaught_exceptions()
 #include <fstream>
@@ -272,10 +272,10 @@ class html_interpolator
             auto const index = std::strtoul(s.c_str() + open_pos + 1, &stop, 
10);

             // Conversion must have stopped at the closing bracket character
-            // and also check for overflow (notice that index == SIZE_MAX
+            // and also check for overflow (notice that index == ULONG_MAX
             // doesn't, in theory, need to indicate overflow, but in practice
             // we're never going to have valid indices close to this number).
-            if(stop != s.c_str() + s.length() - 1 || SIZE_MAX <= index)
+            if(stop != s.c_str() + s.length() - 1 || ULONG_MAX == index)
                 {
                 throw std::runtime_error
                     ("Index of vector variable '" + s + "' is not a valid 
number"
---------------------------------- >8 --------------------------------------

Note that I've also changed "<=" into "==", but we could leave "<=" too and
no warning would still be given when comparing with this constant, which is
always defined appropriately for the size of "index" itself (i.e. 32 bits
under MSW, even in 64 bit builds).

 Please let me know if you see what am I missing, but right now I just
have absolutely no idea why hadn't I done it like this since the beginning.

 And please let me know if you'd prefer me to do a PR with this change or
if you're fine with applying it as is.

 Thanks,
VZ