ICU-22897 Fix memory leak and int overflow #3181
Conversation
1. Rewrite to use LocalPointer to prevent memory leak 2. Rewrite the if/else to switch to make the logic clear 3. Delete the rule if not remember inside the rule set to fix memory leak. 4. Check base value calculation to avoid int64_t overflow. 5. Add memory leak test
FrankYFTang
requested review from
richgillam,
roubert and
grhoten
and removed request for
roubert
| while (p < descriptorLength) { | ||
| c = descriptor.charAt(p); | ||
| if (c >= gZero && c <= gNine) { | ||
| val = val * ll_10 + static_cast<int32_t>(c - gZero); | ||
| int32_t single_digit = static_cast<int32_t>(c - gZero); | ||
| if ((val > 0 && val > (std::numeric_limits<int64_t>::max() - single_digit) / 10) || | ||
| (val < 0 && val < (std::numeric_limits<int64_t>::min() - single_digit) / 10)) { | ||
| // out of int64_t range | ||
| status = U_PARSE_ERROR; | ||
| return; | ||
| } | ||
| val = val * ll_10 + single_digit; |
There was a problem hiding this comment.
Why is the math being done inconsistently here. Why are we using 10 for part of the math and ll_10 for another part of the math?
Where is the test for this if statement? I only see a test for the memory leak.
Wouldn't it be just as easy to check if the sign flipped instead of redundantly performing this math?
There was a problem hiding this comment.
The memory leak test will also cause the usan report int overflow
The sign flipped will happen after int overflow and will cause usan report the error
There was a problem hiding this comment.
Isn't this issue applicable to Java too? This check should be kept in sync with ICU4J.
Can you please use and/or rename ll_10 too? I'm not a fan of magic numbers. The rules are base 10, and it would be helpful to the next maintainer to be more descriptive on why 10 is used.
Checklist