[CDRIVER-3380] Fixup JSON specials and decimal128 parsing #988

vector-of-bool · 2022-05-10T00:44:03Z

Certain token sequences leave the bson-from-json parser unhappy. This addresses those.

vector-of-bool · 2022-05-10T00:44:29Z

src/libbson/src/bson/bson-decimal128.c

@@ -542,7 +542,7 @@ bson_decimal128_from_string_w_len (const char *string,     /* IN */
         continue;
      }

-      if (ndigits_stored < 34) {
+      if (ndigits_stored < BSON_DECIMAL128_MAX_DIGITS) {


No functional change, just replace a magic number.

vector-of-bool · 2022-05-10T00:45:50Z

src/libbson/src/bson/bson-decimal128.c

@@ -634,7 +634,7 @@ bson_decimal128_from_string_w_len (const char *string,     /* IN */
      /* Shift exponent to significand and decrease */
      last_digit++;

-      if (last_digit - first_digit > BSON_DECIMAL128_MAX_DIGITS) {
+      if (last_digit - first_digit >= BSON_DECIMAL128_MAX_DIGITS) {


If last_digit - first_digit is exactly 34 (BSON_DECIMAL128_MAX_DIGITS), we want to treat that as out-of-bounds as well.

vector-of-bool · 2022-05-10T00:47:57Z

src/libbson/src/bson/bson-json.c

+      case BSON_JSON_LF_CODE:
+      case BSON_JSON_LF_DECIMAL128:
+      case BSON_JSON_LF_DOUBLE:
+      case BSON_JSON_LF_INT32:
+      case BSON_JSON_LF_INT64:
+      case BSON_JSON_LF_MAXKEY:
+      case BSON_JSON_LF_MINKEY:
+      case BSON_JSON_LF_OID:
+      case BSON_JSON_LF_OPTIONS:
+      case BSON_JSON_LF_REGEX: // $regex lands here, but $regularExpression does
+                               // not
+      case BSON_JSON_LF_REGULAR_EXPRESSION_OPTIONS:
+      case BSON_JSON_LF_REGULAR_EXPRESSION_PATTERN:
+      case BSON_JSON_LF_SYMBOL:
+      case BSON_JSON_LF_UNDEFINED:
+      case BSON_JSON_LF_UUID:
+         _bson_json_read_set_error (
+            reader,
+            "Unexpected nested object value for \"%s\" key",
+            reader->bson.unescaped.buf);
+         break;


In all of these cases, we are parsing a special "$dollar" object key that does not want a nested sub-object value. Previously, this would fall through and it would later treat the closing brace } of the sub-object as the closing of the entire special object itself. This fixes CDRIVER-3380.

vector-of-bool · 2022-05-10T00:51:20Z

src/libbson/src/jsonsl/jsonsl.c

+                   if (not_u && not_a) {
+                      VERIFY_SPECIAL ("null", 4);
+                   }


This fixes a bug in NaN/null parsing in jsonsl. Previously: Any byte with value 'n' would then be expected to be followed by a u or an A/a character, but this check did not error if 'n' was followed by anything else, and the contiguous sequence of characters would be thrown away and ignored. That is, the string [nope] would parse as an empty array, because the nope would be erroneously ignored. This had fun effects as in {"foo": nope}, where we generate a key with no associated value. Woops.

Great catch. This may be worth submitting a PR to jsonsl.

I considered that, but I also discovered Roberto's existing open PR that has been sitting open for a good while 🙂.

vector-of-bool · 2022-05-10T00:52:41Z

src/libbson/tests/test-json.c

@@ -2727,6 +2727,7 @@ test_bson_json_errors (void)
       "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"},


A test case to reject {"$code": {}}, which does not accept an object value. (Tests CDRIVER-3380)

eramongodb · 2022-05-10T21:09:04Z

src/libbson/src/bson/bson-json.c

         /* special case, we started parsing {$type: {$numberInt: "2"}} and we
          * expected a legacy Binary format. now we see the second "{", so
          * backtrack and parse $type query operator. */
         bson->read_state = BSON_JSON_IN_START_MAP;
+         bson->read_state = BSON_JSON_IN_START_MAP;


Suggested change

bson->read_state = BSON_JSON_IN_START_MAP;

Accidental line duplication?

eramongodb · 2022-05-10T21:09:54Z

src/libbson/src/bson/bson-json.c

+      case BSON_JSON_LF_REGEX: // $regex lands here, but $regularExpression does
+                               // not


What is the significance of this comment?

This is to clear up confusion for future reader, as I was initially confused by this. read_state of BSON_JSON_IN_BSON_TYPE is used when $regex is found, but BSON_JSON_IN_BSON_TYPE_REGEX_STARTMAP is used for $regularExpression, which will instead go to below on line ~1228. They're both called "regex" in their respective enumerators, but they behave differently when parsing.

eramongodb · 2022-05-10T21:12:27Z

src/libbson/src/jsonsl/jsonsl.c

+                   bool not_u = CUR_CHAR != 'u';
+                   bool not_a = tolower (CUR_CHAR) != 'a';


Suggested change

bool not_u = CUR_CHAR != 'u';

bool not_a = tolower (CUR_CHAR) != 'a';

const bool not_u = CUR_CHAR != 'u';

const bool not_a = tolower (CUR_CHAR) != 'a';

eramongodb · 2022-05-10T21:14:09Z

src/libbson/src/jsonsl/jsonsl.c

+                   bool not_u = CUR_CHAR != 'u';
+                   bool not_a = tolower (CUR_CHAR) != 'a';


Minor suggestion: rename and redefine not_u and not_a as is_u and is_a respectively to avoid possibility of double-negatives in conditions (e.g. !not_u).

I used the negation here to maintain the intent of the original code, which is "if the char is not c, unset the bit that corresponds to c". Inverting that would be "if the char is c, do not unset the bit that corresponds to c", which also contains a double negative "do not unset". Storing them as variables is just to dedupe the later && check.

eramongodb · 2022-05-10T21:19:23Z

src/libbson/src/jsonsl/jsonsl.c

+                   if (not_u && not_a) {
+                      VERIFY_SPECIAL ("null", 4);
+                   }


Confirming: is not_u really supposed to be in this conditional? Given we are verifying the value to be "null", the check being conditioned on not_u seems contradictory at first glance.

I forgot to put a comment explaining this. The VERIFY_SPECIAL here will unconditionally generate an error about an invalid "special" token. This is to halt JSON tokenization if a bare n is followed by anything other than a u or an a. The choice of "null" as the string here is just to match what the user more likely meant to type (as NaN is an extension in jsonsl).

eramongodb · 2022-05-11T20:43:32Z

src/libbson/src/jsonsl/jsonsl.c

                      state->special_flags &= ~JSONSL_SPECIALf_NAN;
                   }
+                   if (not_u && not_a) {
+                      /* This veryify will always fail, as we have an 'n'


Suggested change

/* This veryify will always fail, as we have an 'n'

/* This verify will always fail, as we have an 'n'

kevinAlbs

LGTM

kevinAlbs · 2022-05-11T22:20:42Z

src/libbson/src/jsonsl/jsonsl.c

+                   if (not_u && not_a) {
+                      VERIFY_SPECIAL ("null", 4);
+                   }


Great catch. This may be worth submitting a PR to jsonsl.

vector-of-bool added 3 commits May 9, 2022 20:56

Fix JSON parsing of special keys that don't want object values

9cd54a4

Fix handling of invalid specials that begin with 'n'

79b511e

Handle edge case in decimal128 string parsing

8f17485

vector-of-bool commented May 10, 2022

View reviewed changes

vector-of-bool requested review from kevinAlbs and eramongodb May 10, 2022 00:53

vector-of-bool changed the title ~~Fixup JSON specials and decimal128 parsing~~ [CDRIVER-3380] Fixup JSON specials and decimal128 parsing May 10, 2022

eramongodb reviewed May 10, 2022

View reviewed changes

vector-of-bool added 2 commits May 10, 2022 22:11

Comment clarifications and PR tweaks

5950411

Add a missing 'default' label

5c827e4

vector-of-bool requested a review from eramongodb May 11, 2022 18:45

eramongodb approved these changes May 11, 2022

View reviewed changes

spell

3d7b175

kevinAlbs approved these changes May 11, 2022

View reviewed changes

vector-of-bool merged commit c9add86 into mongodb:master May 11, 2022

vector-of-bool deleted the fixup-json-specials branch May 25, 2022 23:40

franmomu mentioned this pull request Jul 17, 2022

Fix failing tests doctrine/mongodb-odm#2452

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CDRIVER-3380] Fixup JSON specials and decimal128 parsing #988

[CDRIVER-3380] Fixup JSON specials and decimal128 parsing #988

vector-of-bool commented May 10, 2022

vector-of-bool May 10, 2022

vector-of-bool May 10, 2022

vector-of-bool May 10, 2022

vector-of-bool May 10, 2022

kevinAlbs May 11, 2022

vector-of-bool May 11, 2022

vector-of-bool May 10, 2022

eramongodb May 10, 2022

eramongodb May 10, 2022

vector-of-bool May 10, 2022

eramongodb May 10, 2022

eramongodb May 10, 2022

vector-of-bool May 10, 2022

eramongodb May 10, 2022

vector-of-bool May 10, 2022

eramongodb May 11, 2022

kevinAlbs left a comment

kevinAlbs May 11, 2022

		case BSON_JSON_LF_REGEX: // $regex lands here, but $regularExpression does
		// not

		bool not_u = CUR_CHAR != 'u';
		bool not_a = tolower (CUR_CHAR) != 'a';

	/* This veryify will always fail, as we have an 'n'
	/* This verify will always fail, as we have an 'n'

[CDRIVER-3380] Fixup JSON specials and decimal128 parsing #988

[CDRIVER-3380] Fixup JSON specials and decimal128 parsing #988

Conversation

vector-of-bool commented May 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinAlbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment