CDRIVER-4662 Check for Decimal128 exponent overflow #1349

joshbsiegel · 2023-07-19T17:42:02Z

Summary

This PR will check that the exponent on a Decimal128 does not overflow when converting from JSON to BSON.

Background

It was brought up here that the C driver does not throw an error when a user tries to convert a Decimal128 value with an exponent that doesn't fit in a 32 bit integer.

What's new

Modified bson_decimal128_from_string to read in the exponent as an int64_t and checking if that number would fit in a 32 bit integer, returning false on failure
Checking the return status of the bson_decimal128_from_string in _bson_json_read_string and setting error on failure
Spec tests added from DRIVERS-2651 Add decimal128 clamped zeros tests with very large exponents. specifications#1432
Test cases for both bson_new_from_json and bson_decimal128_from_string

joshbsiegel · 2023-07-19T20:31:52Z

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


-         if (bson->read_state == BSON_JSON_IN_BSON_TYPE) {
+         if (bson_decimal128_from_string (val_w_null, &decimal128) &&
+             bson->read_state == BSON_JSON_IN_BSON_TYPE) {
            bson->bson_type_data.v_decimal128.value = decimal128;
         } else {
            goto BAD_PARSE;


On failure, BAD_PARSE will set the error message as

Invalid input string "0E+xxx", looking for DECIMAL128

I think this message is fine as is, but potentially could be more helpful to the user with some changes. I believe it would involve adding some return information from bson_decimal128_from_string as to why the parsing fails (because it may not have to do with an overflowing exponent). Open to thoughts.

I think it may help to determine the cause of the error with a new function: bson_decimal128_from_string_w_error. But I think that is separate from this bug fix and could be considered in a future change.

src/libbson/tests/test-json.c

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

kevinAlbs · 2023-07-20T20:10:13Z

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

@@ -581,10 +581,12 @@ bson_decimal128_from_string_w_len (const char *string,     /* IN */
 #else
 #define SSCANF sscanf
 #endif
-      int read_exponent = SSCANF (++str_read, "%d%n", &exponent, &nread);
+      int read_exponent =
+         SSCANF (++str_read, "%" PRId64 "%n", &exponent, &nread);


Please investigate the test failures on Windows.

The definition of SSCANF differs on Windows and may be related to the error.

It ended up being an issue with not converting the INT32 max and min values to int64_t 😑

vector-of-bool

LGTM

kevinAlbs

LGTM with minor changes.

This is a bug fix and I think suitable for a patch release. After merging, please cherry-pick the commit to the r1.24 branch with:

git checkout --track upstream/r1.24
git cherry-pick <commit from master>
git push upstream r1.24

kevinAlbs · 2023-07-24T19:48:21Z

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

+      if (!read_exponent || nread == 0 || temp_exponent > (int64_t) INT32_MAX ||
+          temp_exponent < (int64_t) INT32_MIN) {


Suggested change

if (!read_exponent || nread == 0 || temp_exponent > (int64_t) INT32_MAX ||

temp_exponent < (int64_t) INT32_MIN) {

if (!read_exponent || nread == 0 || !bson_in_range_int32_t_signed (temp_exponent)) {

kevinAlbs · 2023-07-24T19:49:30Z

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

@@ -501,7 +501,7 @@ bson_decimal128_from_string_w_len (const char *string,     /* IN */
   size_t first_digit = 0;           /* The index of the first non-zero digit */
   size_t last_digit = 0;            /* The index of the last digit */

-   int32_t exponent = 0;
+   int64_t exponent = 0;


Suggested change

int64_t exponent = 0;

int32_t exponent = 0;

Change can be reverted since exponent is assigned after casting to int32_t.

kevinAlbs · 2023-07-24T19:55:50Z

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


-         if (bson->read_state == BSON_JSON_IN_BSON_TYPE) {
+         if (bson_decimal128_from_string (val_w_null, &decimal128) &&
+             bson->read_state == BSON_JSON_IN_BSON_TYPE) {
            bson->bson_type_data.v_decimal128.value = decimal128;
         } else {
            goto BAD_PARSE;


I think it may help to determine the cause of the error with a new function: bson_decimal128_from_string_w_error. But I think that is separate from this bug fix and could be considered in a future change.

joshbsiegel added 2 commits July 13, 2023 17:27

add spec tests

0331a34

check for exponent too large, tests

d2a6642

joshbsiegel marked this pull request as draft July 19, 2023 17:42

joshbsiegel changed the title ~~Cdriver 4662~~ CDRIVER-4662 Check for Decimal128 exponent overflow Jul 19, 2023

fix test name

60824e3

joshbsiegel commented Jul 19, 2023

View reviewed changes

joshbsiegel marked this pull request as ready for review July 19, 2023 20:35

joshbsiegel requested a review from kevinAlbs July 19, 2023 20:35

kevinAlbs reviewed Jul 20, 2023

View reviewed changes

joshbsiegel added 10 commits July 21, 2023 16:03

int64 min max tests

5213f41

add int32_t conversion

ab802e4

modfiy from PRId64 to SCNd64

661d578

add print statement

43d6a48

more print statements

928a132

print if statement conditionals and rework ifdef

43e6094

int64 conversions on int32 min and max

7c77480

min and max explicit conversions

6b5eb17

cleaning up

8fc7427

add empty lines back in

fd02d95

joshbsiegel requested a review from kevinAlbs July 21, 2023 22:05

kevinAlbs requested a review from vector-of-bool July 24, 2023 18:04

vector-of-bool approved these changes Jul 24, 2023

View reviewed changes

kevinAlbs approved these changes Jul 24, 2023

View reviewed changes

revert exponent type, use existing range checker

0de9502

joshbsiegel merged commit c3587f1 into mongodb:master Jul 24, 2023

joshbsiegel added a commit that referenced this pull request Jul 24, 2023

CDRIVER-4662 check for decimal128 exponent overflow (#1349)

e50a625

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

CDRIVER-4662 Check for Decimal128 exponent overflow #1349

CDRIVER-4662 Check for Decimal128 exponent overflow #1349

Uh oh!

joshbsiegel commented Jul 19, 2023 •

edited

Loading

Uh oh!

joshbsiegel Jul 19, 2023

Uh oh!

kevinAlbs Jul 24, 2023

Uh oh!

Uh oh!

Uh oh!

kevinAlbs Jul 20, 2023

Uh oh!

joshbsiegel Jul 21, 2023

Uh oh!

vector-of-bool left a comment

Uh oh!

kevinAlbs left a comment

Uh oh!

kevinAlbs Jul 24, 2023

Uh oh!

kevinAlbs Jul 24, 2023

Uh oh!

kevinAlbs Jul 24, 2023

Uh oh!

Uh oh!

		if (!read_exponent \|\| nread == 0 \|\| temp_exponent > (int64_t) INT32_MAX \|\|
		temp_exponent < (int64_t) INT32_MIN) {

	if (!read_exponent \|\| nread == 0 \|\| temp_exponent > (int64_t) INT32_MAX \|\|
	temp_exponent < (int64_t) INT32_MIN) {
	if (!read_exponent \|\| nread == 0 \|\| !bson_in_range_int32_t_signed (temp_exponent)) {

CDRIVER-4662 Check for Decimal128 exponent overflow #1349

CDRIVER-4662 Check for Decimal128 exponent overflow #1349

Uh oh!

Conversation

joshbsiegel commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Background

What's new

Uh oh!

joshbsiegel Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

kevinAlbs Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kevinAlbs Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

joshbsiegel Jul 21, 2023

Choose a reason for hiding this comment

Uh oh!

vector-of-bool left a comment

Choose a reason for hiding this comment

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

kevinAlbs Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

kevinAlbs Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

kevinAlbs Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joshbsiegel commented Jul 19, 2023 •

edited

Loading