[CDRIVER-5859] Enable sign-compare warnings globally, and fix them all (#1856)

* Add a statement macro that can assert SIGABRT

The `mlib_assert_aborts` macro checks that the following statement
terminates the process with SIGABRT. This relies on `fork()`, so it only
works on Unix systems. On Win32 it is a no-op.

* New test assertion macro
* Integer range looping macro.

`mlib_foreach_irange` and `mlib_foreach_urange` provide
concise looping over an integral range.

* Looping over arrays
* Replace all sign-compare loops with macro loops.

This change swaps any `for (...)` loops that trigger
`-Wsign-compare` with `mlib/loop` macros. This also simplifies
redundant code around looping over array elements.

* Integer encoding utilities.

This adds function for reading little-endian i32, u32, i64, and u64 from
pointers to memory. This allows us to decode integers in a single line
instead of doing a declare+memcpy+byteswap that clutters the code and
prevents us from using `const` and correct signedness. Instead, we can
declare and initialize integers of the exact size and sign that we want
in a single line.

* Use integer decoding functions throughout the codebase
* Enable sign-compare warnings globally, and fix every occurrence

Author: vector-of-bool

.clang-format

@@ -105,7 +105,12 @@ ForEachMacros:
   - foreach
   - Q_FOREACH
   - BOOST_FOREACH
+  - mlib_foreach_irange
+  - mlib_foreach_urange
+  - mlib_foreach
+  - mlib_foreach_arr
 IfMacros:
+  - mlib_assert_aborts
   - KJ_IF_MAYBE
 IncludeBlocks:   Preserve
 IncludeCategories:

CMakeLists.txt

@@ -322,6 +322,9 @@ if(ENABLE_MAINTAINER_FLAGS)
       gnu-like:-Wuninitialized
       # Disabled, for now:
       gnu-like:-Wno-strict-aliasing
+      # Sign-comparison-mismatch:
+      gnu-like:-Wsign-compare
+      msvc:/we4018
    )
 endif()
 

src/common/src/common-b64.c

@@ -42,6 +42,7 @@
 
 #include <bson/bson.h>
 #include <common-b64-private.h>
+#include <mlib/loop.h>
 
 #define Assert(Cond) \
    if (!(Cond))      \
@@ -118,21 +119,21 @@ mcommon_b64_ntop (uint8_t const *src, size_t srclength, char *target, size_t tar
    size_t datalength = 0;
    uint8_t input[3];
    uint8_t output[4];
-   size_t i;
 
    if (!target) {
       return -1;
    }
 
-   while (2 < srclength) {
+   // While we have at least three chars to read:
+   while (srclength > 2) {
       input[0] = *src++;
       input[1] = *src++;
       input[2] = *src++;
       srclength -= 3;
 
       output[0] = input[0] >> 2;
-      output[1] = ((input[0] & 0x03) << 4) + (input[1] >> 4);
-      output[2] = ((input[1] & 0x0f) << 2) + (input[2] >> 6);
+      output[1] = (uint8_t) (((input[0] & 0x03) << 4) + (input[1] >> 4));
+      output[2] = (uint8_t) (((input[1] & 0x0f) << 2) + (input[2] >> 6));
       output[3] = input[2] & 0x3f;
       Assert (output[0] < 64);
       Assert (output[1] < 64);
@@ -153,12 +154,10 @@ mcommon_b64_ntop (uint8_t const *src, size_t srclength, char *target, size_t tar
       /* Get what's left. */
       input[0] = input[1] = input[2] = '\0';
 
-      for (i = 0; i < srclength; i++) {
-         input[i] = *src++;
-      }
+      memcpy (input, src, srclength);
       output[0] = input[0] >> 2;
-      output[1] = ((input[0] & 0x03) << 4) + (input[1] >> 4);
-      output[2] = ((input[1] & 0x0f) << 2) + (input[2] >> 6);
+      output[1] = (uint8_t) (((input[0] & 0x03) << 4) + (input[1] >> 4));
+      output[2] = (uint8_t) (((input[1] & 0x0f) << 2) + (input[2] >> 6));
       Assert (output[0] < 64);
       Assert (output[1] < 64);
       Assert (output[2] < 64);
@@ -277,27 +276,24 @@ static const uint8_t mongoc_b64rmap_invalid = 0xff;
 
 static MONGOC_COMMON_ONCE_FUN (bson_b64_initialize_rmap)
 {
-   int i;
-   unsigned char ch;
-
    /* Null: end of string, stop parsing */
    mongoc_b64rmap[0] = mongoc_b64rmap_end;
 
-   for (i = 1; i < 256; ++i) {
-      ch = (unsigned char) i;
+   mlib_foreach_urange (i, 1, 256) {
+      const uint8_t ch = (uint8_t) i;
       /* Whitespaces */
       if (bson_isspace (ch))
-         mongoc_b64rmap[i] = mongoc_b64rmap_space;
+         mongoc_b64rmap[ch] = mongoc_b64rmap_space;
       /* Padding: stop parsing */
       else if (ch == Pad64)
-         mongoc_b64rmap[i] = mongoc_b64rmap_end;
+         mongoc_b64rmap[ch] = mongoc_b64rmap_end;
       /* Non-base64 char */
       else
-         mongoc_b64rmap[i] = mongoc_b64rmap_invalid;
+         mongoc_b64rmap[ch] = mongoc_b64rmap_invalid;
    }
 
    /* Fill reverse mapping for base64 chars */
-   for (i = 0; Base64[i] != '\0'; ++i)
+   for (uint8_t i = 0; Base64[i] != '\0'; ++i)
       mongoc_b64rmap[(uint8_t) Base64[i]] = i;
 
    MONGOC_COMMON_ONCE_RETURN;
@@ -313,7 +309,7 @@ mongoc_b64_pton_do (char const *src, uint8_t *target, size_t targsize)
    tarindex = 0;
 
    while (1) {
-      ch = *src++;
+      ch = (uint8_t) *src++;
       ofs = mongoc_b64rmap[ch];
 
       if (ofs >= mongoc_b64rmap_special) {
@@ -367,22 +363,22 @@ mongoc_b64_pton_do (char const *src, uint8_t *target, size_t targsize)
     * on a byte boundary, and/or with erroneous trailing characters.
     */
 
-   if (ch == Pad64) { /* We got a pad char. */
-      ch = *src++;    /* Skip it, get next. */
+   if (ch == Pad64) {        /* We got a pad char. */
+      ch = (uint8_t) *src++; /* Skip it, get next. */
       switch (state) {
       case 0: /* Invalid = in first position */
       case 1: /* Invalid = in second position */
          return (-1);
 
       case 2: /* Valid, means one byte of info */
          /* Skip any number of spaces. */
-         for ((void) NULL; ch != '\0'; ch = *src++)
+         for ((void) NULL; ch != '\0'; ch = (uint8_t) *src++)
             if (mongoc_b64rmap[ch] != mongoc_b64rmap_space)
                break;
          /* Make sure there is another trailing = sign. */
          if (ch != Pad64)
             return (-1);
-         ch = *src++; /* Skip the = */
+         ch = (uint8_t) *src++; /* Skip the = */
          /* Fall through to "single trailing =" case. */
          /* FALLTHROUGH */
 
@@ -391,7 +387,7 @@ mongoc_b64_pton_do (char const *src, uint8_t *target, size_t targsize)
           * We know this char is an =.  Is there anything but
           * whitespace after it?
           */
-         for ((void) NULL; ch != '\0'; ch = *src++)
+         for ((void) NULL; ch != '\0'; ch = (uint8_t) *src++)
             if (mongoc_b64rmap[ch] != mongoc_b64rmap_space)
                return (-1);
 
@@ -422,14 +418,13 @@ mongoc_b64_pton_do (char const *src, uint8_t *target, size_t targsize)
 static int
 mongoc_b64_pton_len (char const *src)
 {
-   int tarindex, state;
-   uint8_t ch, ofs;
-
-   state = 0;
-   tarindex = 0;
+   uint8_t ch = 0;
+   uint8_t ofs = 0;
+   int state = 0;
+   int tarindex = 0;
 
    while (1) {
-      ch = *src++;
+      ch = (uint8_t) *src++;
       ofs = mongoc_b64rmap[ch];
 
       if (ofs >= mongoc_b64rmap_special) {
@@ -469,22 +464,22 @@ mongoc_b64_pton_len (char const *src)
     * on a byte boundary, and/or with erroneous trailing characters.
     */
 
-   if (ch == Pad64) { /* We got a pad char. */
-      ch = *src++;    /* Skip it, get next. */
+   if (ch == Pad64) {        /* We got a pad char. */
+      ch = (uint8_t) *src++; /* Skip it, get next. */
       switch (state) {
       case 0: /* Invalid = in first position */
       case 1: /* Invalid = in second position */
          return (-1);
 
       case 2: /* Valid, means one byte of info */
          /* Skip any number of spaces. */
-         for ((void) NULL; ch != '\0'; ch = *src++)
+         for ((void) NULL; ch != '\0'; ch = (uint8_t) *src++)
             if (mongoc_b64rmap[ch] != mongoc_b64rmap_space)
                break;
          /* Make sure there is another trailing = sign. */
          if (ch != Pad64)
             return (-1);
-         ch = *src++; /* Skip the = */
+         ch = (uint8_t) *src++; /* Skip the = */
          /* Fall through to "single trailing =" case. */
          /* FALLTHROUGH */
 
@@ -493,7 +488,7 @@ mongoc_b64_pton_len (char const *src)
           * We know this char is an =.  Is there anything but
           * whitespace after it?
           */
-         for ((void) NULL; ch != '\0'; ch = *src++)
+         for (; ch != '\0'; ch = (uint8_t) *src++)
             if (mongoc_b64rmap[ch] != mongoc_b64rmap_space)
                return (-1);
 

src/common/src/common-md5.c

@@ -338,8 +338,8 @@ void
 mcommon_md5_append (bson_md5_t *pms, const uint8_t *data, uint32_t nbytes)
 {
    const uint8_t *p = data;
-   int left = nbytes;
-   int offset = (pms->count[0] >> 3) & 63;
+   uint32_t left = nbytes;
+   uint8_t offset = (pms->count[0] >> 3) & 63;
    uint32_t nbits = (uint32_t) (nbytes << 3);
 
    if (nbytes <= 0)
@@ -353,7 +353,7 @@ mcommon_md5_append (bson_md5_t *pms, const uint8_t *data, uint32_t nbytes)
 
    /* Process an initial partial block. */
    if (offset) {
-      int copy = (offset + nbytes > 64 ? 64 - offset : nbytes);
+      uint32_t copy = (offset + nbytes > 64u ? 64u - offset : nbytes);
 
       memcpy (pms->buf + offset, p, copy);
       if (offset + copy < 64)

src/common/src/common-string.c

@@ -200,8 +200,8 @@ mcommon_string_append_base64_encode (mcommon_string_append_t *append, const uint
    if (encoded_target_len <= (size_t) max_append_len) {
       // No truncation needed. Grow the buffer and encode directly.
       mcommon_string_grow_to_capacity (string, old_len + encoded_target_len);
-      BSON_ASSERT (encoded_target_len ==
-                   mcommon_b64_ntop (bytes, (size_t) len, string->str + old_len, encoded_target_len + 1));
+      const int tgt = mcommon_b64_ntop (bytes, (size_t) len, string->str + old_len, encoded_target_len + 1);
+      BSON_ASSERT (mlib_cmp (encoded_target_len, ==, tgt));
       BSON_ASSERT (mlib_in_range (uint32_t, encoded_target_len));
       string->len = old_len + (uint32_t) encoded_target_len;
       return true;
@@ -227,18 +227,18 @@ mcommon_string_append_base64_encode (mcommon_string_append_t *append, const uint
       uint32_t direct_input_len = mcommon_b64_pton_calculate_target_size ((size_t) direct_encoded_len);
       BSON_ASSERT (direct_input_len % 3 == 0);
       BSON_ASSERT (direct_input_len < len);
-      BSON_ASSERT (direct_encoded_len ==
-                   mcommon_b64_ntop (bytes, (size_t) direct_input_len, string->str + old_len, direct_encoded_len + 1));
+      const int tgt =
+         mcommon_b64_ntop (bytes, (size_t) direct_input_len, string->str + old_len, direct_encoded_len + 1);
+      BSON_ASSERT (mlib_cmp (direct_encoded_len, ==, tgt));
 
       char remainder_buffer[5];
       uint32_t remainder_input_len = BSON_MIN (3, len - direct_input_len);
       BSON_ASSERT (remainder_input_len > 0);
       uint32_t remainder_encoded_len = mcommon_b64_ntop_calculate_target_size ((size_t) remainder_input_len) - 1;
       BSON_ASSERT (remainder_encoded_len > remainder_truncated_len);
-      BSON_ASSERT (remainder_encoded_len == mcommon_b64_ntop (bytes + direct_input_len,
-                                                              (size_t) remainder_input_len,
-                                                              remainder_buffer,
-                                                              sizeof remainder_buffer));
+      const int t2 = mcommon_b64_ntop (
+         bytes + direct_input_len, (size_t) remainder_input_len, remainder_buffer, sizeof remainder_buffer);
+      BSON_ASSERT (mlib_cmp (remainder_encoded_len, ==, t2));
       memcpy (buffer + old_len + direct_encoded_len, remainder_buffer, remainder_encoded_len);
 
       BSON_ASSERT (old_len + direct_encoded_len + remainder_truncated_len == max_len);

src/common/src/mlib/config.h

@@ -175,7 +175,7 @@
 #elif defined(__BYTE_ORDER) && defined(__LITTLE_ENDIAN)
 #define mlib_is_little_endian() (__BYTE_ORDER == __LITTLE_ENDIAN)
 #elif defined(_WIN32)
-#define mlib_is_mlib_is_little_endian() 1
+#define mlib_is_little_endian() 1
 #else
 #error "Do not know how to detect endianness on this platform."
 #endif
@@ -238,6 +238,8 @@
 #define mlib_pragma(...) _Pragma (#__VA_ARGS__) mlib_static_assert (1, "")
 #endif
 
+#define MLIB_FUNC MLIB_IF_GNU_LIKE (__func__) MLIB_IF_MSVC (__FUNCTION__)
+
 #define mlib_diagnostic_push()                           \
    MLIB_IF_GNU_LIKE (mlib_pragma (GCC diagnostic push);) \
    MLIB_IF_MSVC (mlib_pragma (warning (push));)          \