mongo-c-driver icon indicating copy to clipboard operation
mongo-c-driver copied to clipboard

CDRIVER-5732 Replace _bson_append with BSON_APPEND_BYTES

Open eramongodb opened this issue 5 months ago • 1 comments

Summary

Resolves CDRIVER-5732. Verified by this patch.

Replaces the painfully-difficult-to-maintain _bson_append internal variadic function with a new macro-based pattern to avoid a wide range of issues concerning integer arithmetic validation and bounds checks.

These changes also address 49 GCC and 51 Clang unique warnings for -Wimplicit-int-conversion and -Wsign-conversion.

Details

Using bson_append_utf8 as a representative example, replaces the old pattern of invoking _bson_append:

// Abundance of magic numbers.
// Formatted code makes length-value pairing difficult to read.
// Potential signed integer overflow in computation of n_bytes argument.
// Significant va_args parsing complexity and internal arithmetic checks.
return _bson_append (bson,
                     6,
                     (1 + key_length + 1 + 4 + length + 1), // n_bytes
                     1,
                     &type,
                     key_length,
                     key,
                     1,
                     &gZero,
                     4,
                     &length_le,
                     length,
                     value,
                     1, &gZero);

with a new macro-based approach:

// Local state.
BSON_APPEND_BYTES_LIST_DECLARE (args);

// Each additional argument updates the local state.
// No magic numbers or manual arithmetic in usage code.
// Bounds and arithmetic are checked immediately per argument.
BSON_APPEND_BYTES_ADD_ARGUMENT (bson, args, &type, sizeof (type));
BSON_APPEND_BYTES_ADD_CHECKED_STRING (bson, args, key, key_length);
BSON_APPEND_BYTES_ADD_ARGUMENT (bson, args, &gZero, sizeof (gZero));
BSON_APPEND_BYTES_ADD_ARGUMENT (bson, args, &ulength_arg, sizeof (ulength_arg));
BSON_APPEND_BYTES_ADD_ARGUMENT (bson, args, value, ulength);
BSON_APPEND_BYTES_ADD_ARGUMENT (bson, args, &gZero, sizeof (gZero));

// BSON object is only modified after passing all bounds checks above.
BSON_APPEND_BYTES_APPLY_ARGUMENTS (bson, args);

return true;

append_failure:
  return false; // For cleanup opportunity on failure.

The local state constitutes an array of stack-allocated objects that contain the data+length pairs to be appended to the BSON document after all bounds checks have completed. This stack-allocated array fulfills the role of the variadic arguments of the old _bson_append function. However, unlike with _bson_append, bounds checks are computed immediately and inline at the call site with early return behavior on failure (via goto append_failure; to permit cleanup routines). This greatly simplifies error handling and arithmetic computation and improves readability while retaining append-only-on-success behavior (no partial writes).

Additionally audited all computation of lengths and strlen results in associated functions by adding checks to ensure uint32_t representability as well as avoid any potential for signed integer overflow.

eramongodb avatar Sep 23 '24 21:09 eramongodb