Skip to content

CDRIVER-5667 Add missing BSON type checks in special key handlers #1693

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 13, 2024
Merged

CDRIVER-5667 Add missing BSON type checks in special key handlers #1693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c16ac54
Use X macro for special keys
eramongodb Aug 8, 2024
d784ec5
Use consistent key comparison expressions
eramongodb Aug 8, 2024
63f92c7
CDRIVER-5667 Add missing BSON type checks in special key handlers
eramongodb Aug 8, 2024
897909d
Add regression test to /bson/json/errors
eramongodb Aug 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 70 additions & 33 deletions src/libbson/src/bson/bson-json.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,20 +362,27 @@ _noop (void)
return; \
} else \
(void) 0
#define HANDLE_OPTION(_selection_statement, _key, _type, _state) \
_selection_statement (len == strlen (_key) && strncmp ((const char *) val, (_key), len) == 0) \
{ \
if (bson->bson_type && bson->bson_type != (_type)) { \
_bson_json_read_set_error (reader, \
"Invalid key \"%s\". Looking for values " \
"for type \"%s\", got \"%s\"", \
(_key), \
_bson_json_type_name (bson->bson_type), \
_bson_json_type_name (_type)); \
return; \
} \
bson->bson_type = (_type); \
bson->bson_state = (_state); \

#define HANDLE_OPTION_KEY_COMPARE(_key) (len == strlen (_key) && memcmp (key, (_key), len) == 0)

#define HANDLE_OPTION_TYPE_CHECK(_key, _type) \
if (bson->bson_type && bson->bson_type != (_type)) { \
_bson_json_read_set_error (reader, \
"Invalid key \"%s\". Looking for values " \
"for type \"%s\", got \"%s\"", \
(_key), \
_bson_json_type_name (bson->bson_type), \
_bson_json_type_name (_type)); \
return; \
} \
((void) 0)
Comment on lines +377 to +378
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be an else before the cast-to-void?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I omitted the else because it didn't appear to be necessary. The if is scoped with {}, so there is (AFAIK) no opportunity for else hijacking or other (plausible) mistakes which may produce unexpected behavior at usage sites. A plain ((void)0) seems sufficient to force a semicolon regardless of the macro definition (if or otherwise). If I am incorrect and it is advisable to keep the else anyways, a followup PR can patch it in.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is so contained, I don't think there's any risk of anything goofy happening. My only thought was about:

if (x)
    HANDLE_OPTION_TYPE_CHECK(...)
else
    lol();

But that would just be a hard compile-error anyway.


#define HANDLE_OPTION(_selection_statement, _key, _type, _state) \
_selection_statement (HANDLE_OPTION_KEY_COMPARE (_key)) \
{ \
HANDLE_OPTION_TYPE_CHECK (_key, _type); \
bson->bson_type = (_type); \
bson->bson_state = (_state); \
}


Expand Down Expand Up @@ -1159,23 +1166,40 @@ _bson_json_read_start_map (bson_json_reader_t *reader) /* IN */
}


#define BSON_PRIVATE_SPECIAL_KEYS_XMACRO(X) \
X (binary) \
X (code) \
X (date) \
X (dbPointer) \
X (maxKey) \
X (minKey) \
X (numberDecimal) \
X (numberDouble) \
X (numberInt) \
X (numberLong) \
X (oid) \
X (options) \
X (regex) \
X (regularExpression) \
X (scope) \
X (symbol) \
X (timestamp) \
X (type) \
X (undefined) \
X (uuid)


static bool
_is_known_key (const char *key, size_t len)
{
bool ret;

#define IS_KEY(k) (len == strlen (k) && (0 == memcmp (k, key, len)))

ret = (IS_KEY ("$regularExpression") || IS_KEY ("$regex") || IS_KEY ("$options") || IS_KEY ("$code") ||
IS_KEY ("$scope") || IS_KEY ("$oid") || IS_KEY ("$binary") || IS_KEY ("$type") || IS_KEY ("$date") ||
IS_KEY ("$undefined") || IS_KEY ("$maxKey") || IS_KEY ("$minKey") || IS_KEY ("$timestamp") ||
IS_KEY ("$numberInt") || IS_KEY ("$numberLong") || IS_KEY ("$numberDouble") || IS_KEY ("$numberDecimal") ||
IS_KEY ("$numberInt") || IS_KEY ("$numberLong") || IS_KEY ("$numberDouble") || IS_KEY ("$numberDecimal") ||
IS_KEY ("$dbPointer") || IS_KEY ("$symbol") || IS_KEY ("$uuid"));

#define IS_KEY(k) \
if (len == strlen ("$" #k) && (0 == memcmp ("$" #k, key, len))) { \
return true; \
}
BSON_PRIVATE_SPECIAL_KEYS_XMACRO (IS_KEY)
#undef IS_KEY

return ret;
return false;
}

static void
Expand Down Expand Up @@ -1242,9 +1266,10 @@ _bson_json_read_map_key (bson_json_reader_t *reader, /* IN */
return;
}

const char *const key = (const char *) val;

if (bson->read_state == BSON_JSON_IN_START_MAP) {
if (len > 0 && val[0] == '$' && _is_known_key ((const char *) val, len) &&
bson->n >= 0 /* key is in subdocument */) {
if (len > 0 && key[0] == '$' && _is_known_key (key, len) && bson->n >= 0 /* key is in subdocument */) {
bson->read_state = BSON_JSON_IN_BSON_TYPE;
bson->bson_type = (bson_type_t) 0;
memset (&bson->bson_type_data, 0, sizeof bson->bson_type_data);
Expand Down Expand Up @@ -1281,30 +1306,42 @@ _bson_json_read_map_key (bson_json_reader_t *reader, /* IN */
HANDLE_OPTION (else if, "$numberDouble", BSON_TYPE_DOUBLE, BSON_JSON_LF_DOUBLE)
HANDLE_OPTION (else if, "$symbol", BSON_TYPE_SYMBOL, BSON_JSON_LF_SYMBOL)
HANDLE_OPTION (else if, "$numberDecimal", BSON_TYPE_DECIMAL128, BSON_JSON_LF_DECIMAL128)
else if (!strcmp ("$timestamp", (const char *) val))
else if (HANDLE_OPTION_KEY_COMPARE ("$timestamp"))
{
HANDLE_OPTION_TYPE_CHECK ("$timestamp", BSON_TYPE_TIMESTAMP);
bson->bson_type = BSON_TYPE_TIMESTAMP;
bson->read_state = BSON_JSON_IN_BSON_TYPE_TIMESTAMP_STARTMAP;
}
else if (!strcmp ("$regularExpression", (const char *) val))
else if (HANDLE_OPTION_KEY_COMPARE ("$regularExpression"))
{
HANDLE_OPTION_TYPE_CHECK ("$regularExpression", BSON_TYPE_REGEX);
bson->bson_type = BSON_TYPE_REGEX;
bson->read_state = BSON_JSON_IN_BSON_TYPE_REGEX_STARTMAP;
}
else if (!strcmp ("$dbPointer", (const char *) val))
else if (HANDLE_OPTION_KEY_COMPARE ("$dbPointer"))
{
HANDLE_OPTION_TYPE_CHECK ("$dbPointer", BSON_TYPE_DBPOINTER);

/* start parsing "key": {"$dbPointer": {...}}, save "key" for later */
_bson_json_buf_set (&bson->dbpointer_key, bson->key, bson->key_buf.len);

bson->bson_type = BSON_TYPE_DBPOINTER;
bson->read_state = BSON_JSON_IN_BSON_TYPE_DBPOINTER_STARTMAP;
}
else if (!strcmp ("$code", (const char *) val))
else if (HANDLE_OPTION_KEY_COMPARE ("$code"))
{
// "$code" may come after "$scope".
if (bson->bson_type != BSON_TYPE_CODEWSCOPE) {
HANDLE_OPTION_TYPE_CHECK ("$code", BSON_TYPE_CODE);
}
_bson_json_read_code_or_scope_key (bson, false /* is_scope */, val, len);
}
else if (!strcmp ("$scope", (const char *) val))
else if (HANDLE_OPTION_KEY_COMPARE ("$scope"))
{
// "$scope" may come after "$code".
if (bson->bson_type != BSON_TYPE_CODE) {
HANDLE_OPTION_TYPE_CHECK ("$scope", BSON_TYPE_CODEWSCOPE);
}
_bson_json_read_code_or_scope_key (bson, true /* is_scope */, val, len);
}
else
Expand Down
14 changes: 8 additions & 6 deletions src/libbson/tests/test-json.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2551,12 +2551,14 @@ test_bson_json_errors (void)
{
typedef const char *test_bson_json_error_t[2];
test_bson_json_error_t tests[] = {
{"{\"x\": {\"$numberLong\": 1}}", "Invalid state for integer read: INT64"},
{"{\"x\": {\"$binary\": 1}}", "Unexpected integer 1 in type \"binary\""},
{"{\"x\": {\"$numberInt\": true}}", "Invalid read of boolean in state IN_BSON_TYPE"},
{"{\"x\": {\"$dbPointer\": true}}", "Invalid read of boolean in state IN_BSON_TYPE_DBPOINTER_STARTMAP"},
{"[{\"$code\": {}}]", "Unexpected nested object value for \"$code\" key"},
{"{\"x\": {\"$numberInt\": \"8589934592\"}}", "Invalid input string \"8589934592\", looking for INT32"},
{BSON_STR ({"x" : {"$numberLong" : 1}}), "Invalid state for integer read: INT64"},
{BSON_STR ({"x" : {"$binary" : 1}}), "Unexpected integer 1 in type \"binary\""},
{BSON_STR ({"x" : {"$numberInt" : true}}), "Invalid read of boolean in state IN_BSON_TYPE"},
{BSON_STR ({"x" : {"$dbPointer" : true}}), "Invalid read of boolean in state IN_BSON_TYPE_DBPOINTER_STARTMAP"},
{BSON_STR ([ {"$code" : {}} ]), "Unexpected nested object value for \"$code\" key"},
{BSON_STR ([ {"$scope" : {}, "$dbPointer" : {"" : {"$code" : ""}}} ]),
"Invalid key \"$dbPointer\". Looking for values for type \"code\", got \"dbpointer\""},
{BSON_STR ({"x" : {"$numberInt" : "8589934592"}}), "Invalid input string \"8589934592\", looking for INT32"},
{0},
};

Expand Down