Prevent fract and rate from overflowing by exiting early #494
Conversation
Signed-off-by: Rose <gfunni234@gmail.com>
| @@ -51,6 +51,7 @@ int __t2q_get_rate(const char **text,int up) | |||
| } | |||
| end++; | |||
| } | |||
| if (power < 0 && fract > INT_MAX / 10) return RATE_ERROR; | |||
There was a problem hiding this comment.
Ummm, shouldn't this be "power > 0" not "<"?
When power is negative we're dividing fract by 10 so there is no possibility of overflow. It's when power is positive that there is potentially some chance of overflow (though not in practice, see below).
In any case, the logic is such that fract can be at most 999999999 after the loop that follows here, since G (giga) is the largest suffix implemented in "mult". That won't overflow on machines with >= 32-bit ints; if you're worried about machines with smaller ints (which I don't consider that this project supports) then the previous loop at line 40 has the possibility of overflow too.
| @@ -69,9 +71,9 @@ int __t2q_get_rate(const char **text,int up) | |||
| rate = (rate+(up ? 8*ATM_CELL_PAYLOAD-1 : 0))/8/ | |||
| ATM_CELL_PAYLOAD; | |||
| end += 3; | |||
| if (rate > INT_MAX) return RATE_ERROR; | |||
There was a problem hiding this comment.
Is this going to work? Can rate ever be greater than INT_MAX? I think the compiler would be justified in removing this test as being obviously always false. Any overflow that could happen would have already happened by this point.
The only overflow that could occur in line 69 is in the addition of the rounding-up constant, because we are basically dividing by 384 here. So perhaps we should instead divide by 8 * ATM_CELL_PAYLOAD and then increment if there was any remainder and 'up' is true. Or something like that. I think it would be wrong to return an error in the case where rate > INT_MAX - 383 and 'up' is true, since the correct result is perfectly well defined and is less than INT_MAX.
|
@AtariDreams: Have you seen the @paulusmack comments? |
|
I did the change I suggested above to avoid any possibility of overflow in the division with rounding up. As I said, I don't think the other change is necessary. Hence I'm closing this PR. |
No description provided.