Fixed UB (requires performance testing) #3569

dbms/src/IO/ReadHelpers.h

@@ -254,7 +254,7 @@ ReturnType readIntTextImpl(T & x, ReadBuffer & buf)
     static constexpr bool throw_exception = std::is_same_v<ReturnType, void>;
 
     bool negative = false;
-    x = 0;
+    std::make_unsigned_t<T> res = 0;
     if (buf.eof())
     {
         if constexpr (throw_exception)
@@ -290,22 +290,17 @@ ReturnType readIntTextImpl(T & x, ReadBuffer & buf)
             case '7': [[fallthrough]];
             case '8': [[fallthrough]];
             case '9':
-                x *= 10;
-                x += *buf.position() - '0';
+                res *= 10;
+                res += *buf.position() - '0';
                 break;
             default:
-                if (negative)
-                    x = -x;
+                x = negative ? -res : res;
                 return ReturnType(true);
         }
         ++buf.position();
     }
 
-    /// NOTE Signed integer overflow is undefined behaviour. Consider we have '128' that is parsed as Int8 and overflowed.
-    /// We are happy if it is overflowed to -128 and then 'x = -x' does nothing. But UBSan will warn.
-    if (negative)
-        x = -x;
-
+    x = negative ? -res : res;
     return ReturnType(true);
 }
 
@@ -331,7 +326,7 @@ template <typename T, bool throw_on_error = true>
 void readIntTextUnsafe(T & x, ReadBuffer & buf)
 {
     bool negative = false;
-    x = 0;
+    std::make_unsigned_t<T> res = 0;
 
     auto on_error = []
     {
@@ -365,8 +360,8 @@ void readIntTextUnsafe(T & x, ReadBuffer & buf)
 
         if ((*buf.position() & 0xF0) == 0x30) /// It makes sense to have this condition inside loop.
         {
-            x *= 10;
-            x += *buf.position() & 0x0F;
+            res *= 10;
+            res += *buf.position() & 0x0F;
             ++buf.position();
         }
         else
@@ -374,8 +369,7 @@ void readIntTextUnsafe(T & x, ReadBuffer & buf)
     }
 
     /// See note about undefined behaviour above.
-    if (std::is_signed_v<T> && negative)
-        x = -x;
+    x = std::is_signed_v<T> && negative ? -res : res;
 }
 
 template <typename T>

dbms/src/IO/WriteIntText.h

@@ -185,7 +185,6 @@ namespace detail
         writeUIntText(static_cast<std::make_unsigned_t<T>>(x), buf);
     }
 
-#if 1
     inline void writeSIntText(__int128 x, WriteBuffer & buf)
     {
         static const __int128 min_int128 = __int128(0x8000000000000000ll) << 64;
@@ -204,7 +203,6 @@ namespace detail
 
         writeUIntText(static_cast<unsigned __int128>(x), buf);
     }
-#endif
 }
 
 

libs/libcommon/src/JSON.cpp

@@ -56,7 +56,7 @@ static UInt64 readUIntText(const char * buf, const char * end)
 static Int64 readIntText(const char * buf, const char * end)
 {
     bool negative = false;
-    Int64 x = 0;
+    UInt64 x = 0;
 
     if (buf == end)
         throw JSONException("JSON: cannot parse signed integer: unexpected end of data.");
@@ -90,10 +90,8 @@ static Int64 readIntText(const char * buf, const char * end)
         }
         ++buf;
     }
-    if (negative)
-        x = -x;
 
-    return x;
+    return negative ? -x : x;
 }
 
 

libs/libmysqlxx/src/Value.cpp

@@ -44,7 +44,7 @@ UInt64 Value::readUIntText(const char * buf, size_t length) const
 Int64 Value::readIntText(const char * buf, size_t length) const
 {
     bool negative = false;
-    Int64 x = 0;
+    UInt64 x = 0;
     const char * end = buf + length;
 
     while (buf != end)
@@ -74,10 +74,8 @@ Int64 Value::readIntText(const char * buf, size_t length) const
         }
         ++buf;
     }
-    if (negative)
-        x = -x;
 
-    return x;
+    return negative ? -x : x;
 }