BLI: Prevent Overflow in Grouped Number Output #105263

Harley Acheson · 2023-02-27T18:59:12+01:00

Harley Acheson commented

2023-02-27 18:59:12 +01:00

Increase the buffer sizes used for BLI_str_format_uint64_grouped to
prevent overflow on strings representing numbers within the uint64.
Also creates and uses defines for all the formatted string buffer
sizes.

When displaying large numbers we often use BLI_str_format_uint64_grouped to add commas as thousands separators. In all the places that we call this, and internal to that function, we use 16-byte characters arrays. This means that it can successfully turn "999999999999" into "999,999,999,999", but will overflow on one trillion or greater as that would require an 18-byte buffer.

This patch increases the buffer size used by this function to 27. And also uses a define for this size, and also for those used by BLI_str_format_byte_unit, BLI_str_format_decimal_unit, and BLI_str_format_integer_unit.

Increase the buffer sizes used for `BLI_str_format_uint64_grouped` to prevent overflow on strings representing numbers within the uint64. Also creates and uses defines for all the formatted string buffer sizes. --- When displaying large numbers we often use `BLI_str_format_uint64_grouped` to add commas as thousands separators. In all the places that we call this, and internal to that function, we use 16-byte characters arrays. This means that it can successfully turn "999999999999" into "999,999,999,999", but will **overflow** on one trillion or greater as that would require an 18-byte buffer. This patch increases the buffer size used by this function to 27. And also uses a define for this size, and also for those used by `BLI_str_format_byte_unit`, `BLI_str_format_decimal_unit`, and `BLI_str_format_integer_unit`.

StatsOverflow.jpg

316 KiB

❤️ 1 🚀 2

Harley Acheson requested review from Campbell Barton 2023-02-27 19:00:27 +01:00

Campbell Barton requested changes 2023-03-01 08:41:23 +01:00

Campbell Barton left a comment

Showing ? seems like a bug from a user perspective, if this were unavoidable it could be an acceptable fallback however increasing the default buffer size to fit the largest uint64_t is preferable.

A quick check shows len("{:3,d}".format(0xffff_ffff_ffff_ffff)) is 26, so the size of char dst[16] for BLI_str_format_uint64_grouped should be increased to 27.

To avoids hard coded values being duplicated, defines can be added to BLI_string.h.

#define BLI_STR_FORMAT_UINT64_GROUPED_SIZE 27
#define BLI_STR_FORMAT_INT32_GROUPED_SIZE 16

Showing `?` seems like a bug from a user perspective, if this were unavoidable it could be an acceptable fallback however increasing the default buffer size to fit the largest `uint64_t` is preferable. A quick check shows `len("{:3,d}".format(0xffff_ffff_ffff_ffff))` is 26, so the size of `char dst[16]` for `BLI_str_format_uint64_grouped` should be increased to 27. To avoids hard coded values being duplicated, defines can be added to `BLI_string.h`. ``` #define BLI_STR_FORMAT_UINT64_GROUPED_SIZE 27 #define BLI_STR_FORMAT_INT32_GROUPED_SIZE 16 ```

Harley Acheson force-pushed GroupedOverflow from eb701771dd to 583bdd73e3

2023-03-01 19:59:13 +01:00

Compare

Harley Acheson commented

2023-03-01 20:03:53 +01:00

@ideasman42 - ...increasing the default buffer size.. defines can be added

Yes, that works pretty well. About the only "complication" is that we quite often use a combination of these formatted strings so it is hard to do just grouped and not do the byte, unit, and integer formats as well. So this version does that too. Doesn't seem to make too much of a mess. I think its better...

> @ideasman42 - ...increasing the default buffer size.. defines can be added Yes, that works pretty well. About the only "complication" is that we quite often use a combination of these formatted strings so it is hard to do just grouped and not do the byte, unit, and integer formats as well. So this version does that too. Doesn't seem to make too much of a mess. I think its better...

Campbell Barton approved these changes 2023-03-01 23:07:26 +01:00

Campbell Barton force-pushed GroupedOverflow from 583bdd73e3 to 39482f0bd9

2023-03-01 23:08:55 +01:00

Compare

Campbell Barton approved these changes 2023-03-01 23:23:54 +01:00

Campbell Barton left a comment

Looks good, it also makes sense to have tests these functions are working as expected (passing in min/max for each type, ensuring values aren't clipped).

source/blender/blenlib/BLI_string.h

						
				@ -20,0 +31,4 @@

				#define BLI_STR_FORMAT_UINT64_DECIMAL_UNIT_SIZE 7

				/* Buffer size of maximum `int32` formatted as very short decimal size ("15B" for example). */

				#define BLI_STR_FORMAT_UINT64_INTEGER_UNIT_SIZE 5

Campbell Barton commented

2023-03-01 23:22:14 +01:00

Should be BLI_STR_FORMAT_INT32_INTEGER_UNIT_SIZE

Harley marked this conversation as resolved

Harley Acheson changed title from ~~BLI: Prevent Overflow in Grouped Number Output~~ to WIP BLI: Prevent Overflow in Grouped Number Output

2023-03-02 00:17:48 +01:00

Harley Acheson commented

2023-03-02 00:20:29 +01:00

Harley: Fix those constants

and update the tests (BLI_string_text.cc). Not sure if all the functions are tested. make sure they all test min/max and use the constants for buffer sizes

Harley: Fix those constants and update the tests (BLI_string_text.cc). Not sure if all the functions are tested. make sure they all test min/max and use the constants for buffer sizes

Harley Acheson force-pushed GroupedOverflow from 39482f0bd9 to e410112e30

2023-03-02 02:22:22 +01:00

Compare

Harley Acheson changed title from ~~WIP BLI: Prevent Overflow in Grouped Number Output~~ to BLI: Prevent Overflow in Grouped Number Output

2023-03-02 02:23:30 +01:00

Harley Acheson commented

2023-03-02 02:26:03 +01:00

@ideasman42 - I fixed those constants for size/sign. And added tests for BLI_str_format_uint64_grouped and made the rest of them use the new constants.

@ideasman42 - I fixed those constants for size/sign. And added tests for `BLI_str_format_uint64_grouped` and made the rest of them use the new constants.

Campbell Barton approved these changes 2023-03-02 10:13:41 +01:00

Harley Acheson added 1 commit 2023-03-02 17:09:42 +01:00