Fixed some potential integer overflows in coreclr#124047
Fixed some potential integer overflows in coreclr#124047tpa95 wants to merge 9 commits intodotnet:mainfrom
Conversation
In the MCList::processArgAsMCL function, an input string consisting of digits and the characters ',' and '-' is parsed into a set of numbers and/or ranges of numbers. If the string contains a number greater than UINT_MAX, the scratch variable will overflow. The input string often comes from command-line arguments, so protection is necessary.
The result of the multiplication will be of type uint32_t, since the highest operand type is uint32_t, so overflow will occur if num_heaps > 255. To promote the result to size_t, you must explicitly convert at least one of the operands to size_t.
| unsigned digit = (*head) - '0'; | ||
| if (scratch > (UINT_MAX - digit) / 10) | ||
| { | ||
| LogError("Invalid int value in '%s'", input); | ||
| return false; | ||
| } | ||
| scratch = (scratch * 10) + digit; |
There was a problem hiding this comment.
This will be superseded by #121367. Superpmi is a development-only tool and doesn't need to be robust for invalid inputs.
There was a problem hiding this comment.
@tpa95 Can you please address the comment here? This shouldn't be changed.
There was a problem hiding this comment.
@huoyaoyuan Sorry, I didn't immediately realize you were the owner and what you meant.
Do you want to remove only the first edit, or both?
There was a problem hiding this comment.
I'm not really the owner but contributing to it. All changes in superpmi should be avoided since it's on the way of being refactored.
|
|
||
| size_t total_new_allocation = new_allocation * num_heaps; | ||
| size_t total_min_allocation = MIN_YOUNGEST_GEN_DESIRED * num_heaps; | ||
| size_t total_min_allocation = (size_t)MIN_YOUNGEST_GEN_DESIRED * num_heaps; |
There was a problem hiding this comment.
The maximum of this product is 16M(MIN_YOUNGEST_GEN_DESIRED) * 1024 (MAX_SUPPORTED_CPUS) = 16G. The multiplication can overflow when >128 heaps are used.
|
Tagging subscribers to this area: @agocke, @dotnet/gc |
|
Hi @agocke @dotnet/gc. Could you please take a look when you have a moment and let me know if this is ready to merge or if you’d like changes? Thanks! |
Fixed some potential integer overflows in gc.cpp and mclist.cpp. The errors were discovered using the Svace static analyzer. I didn't observe any in practice.
In
gc_heap::joined_youngest_desiredthe result of the multiplication will be of typeuint32_t, since the highest operand type isuint32_t, so overflow will occur ifnum_heaps > 255. To promote the result tosize_t, you must explicitly convert at least one of the operands tosize_t.In the
MCList::processArgAsMCL, an input string consisting of digits and the characters ',' and '-' is parsed into a set of numbers and/or ranges of numbers. If the string contains a number greater thanUINT_MAX, thescratchwill overflow. Theinputstring often comes from command-line arguments, so protection is necessary.Found by Linux Verification Center (linuxtesting.org) with SVACE.
Reporter: Pavel Tikhomirov (Tihomirov-P@gaz-is.ru).
Organization: Gazinformservice (resp@gaz-is.ru).