FS#4747 - Fix memory allocation size overflows

Attached to Project: OpenTTD
Opened by Matt D. (monoid) - Thursday, 25 August 2011, 15:16 GMT
Last edited by Michael Lutz (michi_cc) - Friday, 02 September 2011, 20:16 GMT
Type Patch
Category Core
Status Closed
Assigned To No-one
Operating System All
Severity Medium
Priority Normal
Reported Version trunk
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


There are several locations where the size of memory to be allocated uses a calculation that can overflow, which can lead to a smaller amount of memory than expected to be allocated, allowing possible heap modification. Indeed, this is currently possible with a crafted BMP image of certain dimensions.

The allocation helper routines in alloc_func.hpp alone suffer from this flaw; namely in the calculation of num_elements * sizeof(T).

This patch attempts to fix this by:
* Checking the internal size calculations in alloc_func.hpp helpers for overflow
* Changing the size parameter for these helpers from size_t to int64, which allows the use of OverflowSafeInt64's in the calculations of other sizes
* Using OverflowSafeInt64s as just mentioned, in size calculations that maybe/are vulnerable to overflow.
* A few size calculations are manually checked.

I have attempted to keep the changes to usages of the alloc_func.hpp helpers minimal but still correct; variables involved in relevant size calculations are simply wrapped with OSI64() (a typedef for OverflowSafeInt64). However, this is still a decently sized patch, and am open to suggestions on how to rework it differently.
This task depends upon

Closed by  Michael Lutz (michi_cc)
Friday, 02 September 2011, 20:16 GMT
Reason for closing:  Implemented
Additional comments about closing:  In r22873 and r22874.
Comment by Remko Bijker (Rubidium) - Thursday, 25 August 2011, 16:30 GMT
Under which circumstances can either of the width or height component of a resolution become more than UINT16 max? The setting is truncated to UINT16_MAX when being read from the configuration file.

Under which circumstances can the width or height component of the chatbox's size become more than UINT16 max? The width is limited to UINT16 max by both its type as the setting validation, the height is limited by the UINT8 max (type and setting validation) of the number of lines setting, and the height of a line is limited by a setting validation to 72. So where would this overflow.

The BMP bit I can agree on, but it might be easier to reject such files just outright. They are not useful; just diallow anything above 16384x16384 would suffice I'd say.

SIZE_MAX is a size_t (unsigned long). Casting it to int64 on x64 makes it effectively -1, disallowing all allocations on x64. If there is an alloca that would be anywhere near SIZE_MAX, that would be very bad. You can never allocate that much. The stack is a few MB on Windows.

So what I am seeing is just a generic: oh, might be wrong so replace it with something that looks much better... but it's actually pointless and makes the code harder to read.

Comment by Remko Bijker (Rubidium) - Friday, 26 August 2011, 17:23 GMT
More detailed comments per fix. In my opinion it is much better to document this and put some limitations on (primarily) image loading. Then most, if not all, will never overflow.
Comment by Matt D. (monoid) - Saturday, 27 August 2011, 11:30 GMT
I agree, I'm not sure what I was doing with so many changes, and forgetting about 64 bit archs.

I took your detailed comments to produce a more focused patch.
The only bit I wasn't entirely sure about was the Squirrel array GetParam helper - that should be checked by someone in the know.
Comment by Matt D. (monoid) - Saturday, 27 August 2011, 15:07 GMT
Added a few more checks - the original sound in-file size is a dword read straight from the file, so can easily overflow.