[flang][runtime] Fix integer overflow check in FORMAT #79368
Conversation
llvmbot
added
flang:runtime
flang
|
@llvm/pr-subscribers-flang-semantics @llvm/pr-subscribers-flang-runtime Author: Peter Klausler (klausler) ChangesThe integer overflow check used for repeat counts &c. in FORMAT specifications was incorrect; fix it. Fixes #79255. Full diff: https://github.com/llvm/llvm-project/pull/79368.diff 1 Files Affected:
diff --git a/flang/runtime/format-implementation.h b/flang/runtime/format-implementation.h
index c54ac062c7beab9..e475508b9ba58a7 100644
--- a/flang/runtime/format-implementation.h
+++ b/flang/runtime/format-implementation.h
@@ -85,8 +85,9 @@ int FormatControl<CONTEXT>::GetIntField(
ch = PeekNext();
}
while (ch >= '0' && ch <= '9') {
- if (result >
- std::numeric_limits<int>::max() / 10 - (static_cast<int>(ch) - '0')) {
+ if (result > std::numeric_limits<int>::max() / 10 ||
+ std::numeric_limits<int>::max() - 10 * result <
+ static_cast<int>(ch) - '0') {
handler.SignalError(
IostatErrorInFormat, "FORMAT integer field out of range");
if (hadError) {
|
|
Compare format.h lines 206-236. Would that code work here? Does that code also need to be changed? |
The integer overflow check used for repeat counts &c. in FORMAT specifications was incorrect; fix it. Fixes llvm#79255.
Good point. I've reworked the change. |
| lastCursor = cursor_; | ||
| integerValue_ = 10 * integerValue_ + c - '0'; | ||
| if (lastValue > integerValue_) { | ||
| if (integerValue_ > std::numeric_limits<int>::max()) { |
There was a problem hiding this comment.
How will this new condition ever be true?
The idea is that an n+1 digit unsigned number should always be larger than an n digit unsigned number. With bounded computer arithmetic that fails if the number overflows.
There was a problem hiding this comment.
integerValue_ is std::int64_t
There was a problem hiding this comment.
Yes. So what should be a positive value becomes negative on overflow. If the type were unsigned, the overflow value would be positive but smaller. I think both signed and unsigned underlying types have this property. (It's been a long time since I looked at this in detail.)
There was a problem hiding this comment.
integerValue_ hasn't overflowed its 64-bit range, and can be compared to the max 32-bit value safely.
There was a problem hiding this comment.
Why restrict values to 32 bits? Don't we accommodate 64 bit values by "default"?
There was a problem hiding this comment.
The checker in format-implementation.h limited things to 32 bits already. That seems sufficient to me for repeat counts, field widths, &c. in FORMAT strings.
There was a problem hiding this comment.
So the two cases are not then strictly comparable. Maybe the best choice is to leave the compile time format.h code as is and go with new code in the runtime.
The integer overflow check used for repeat counts &c. in FORMAT specifications was incorrect; fix it.
Fixes #79255.