fix possible integer overflow in alloc() #109
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
schmonz
reviewed
Dec 27, 2019
DerDakon
force-pushed
the
Dakon-alloc-align
branch
from
0a091a7 to
8a1517c
Compare
schmonz
reviewed
Dec 27, 2019
mbhangui
approved these changes
Dec 27, 2019
DerDakon
force-pushed
the
Dakon-alloc-align
branch
from
45ced5e to
67bc668
Compare
If n is close to 2**32 then the alignment calculation could cause an integer overflow. Rule this out by checking if the allocation is smaller than the fixed buffer. It could be done by checking against the possible space, which is done later anyway, but checking against SPACE has the benefit that this is a constant.
DerDakon
force-pushed
the
Dakon-alloc-align
branch
from
67bc668 to
75a935b
Compare
schmonz
approved these changes
Dec 27, 2019
This was referenced Dec 27, 2019
Closed
DerDakon
added a commit
that referenced
this pull request
Dec 27, 2019
* it's as simple as possible, probably intentional. This makes things like alloc_re() (i.e. realloc()) inefficient as it does not look if the thing it reallocates is already the last thing in the batch, and simply extends that, or if alloc_free() has the last block and adds things back to the pool (ideally with zeroing for security reasons). * it makes it basically impossible to detect overruns on early small allocations with tools like valgrind, address sanitizer or friends as those allocations never hit malloc(), so are never traced by any custom tools. Any overrun will just go into the next variable, untracable. * allocators do their own buffering, alignment, and so on. If they are broken, your system is severely screwed. And if we need more money^H^H^H^Hemory we will hit that buggy allocatator anyway, making things harder to detect as it will only happen on "heavy" memory usage. This is not TruOS, SunOS 1.x or something like that anymore. We expect a reasonably sane OS. Everything else will get us screwed in much more severe ways. * it has shown issues when coming close to 4GiB allocations (#37, #109, older reports). DJB correctly says that an allocation of that size in your mailer shows that you have already done something wrong, but still: 64 bit platforms are the default, so using a 32 bit type to pass allocation sizes is a bug of it's own. * why bother at all with having a custom allocator?
DerDakon
added a commit
that referenced
this pull request
Dec 29, 2019
* it's as simple as possible, probably intentional. This makes things like alloc_re() (i.e. realloc()) inefficient as it does not look if the thing it reallocates is already the last thing in the batch, and simply extends that, or if alloc_free() has the last block and adds things back to the pool (ideally with zeroing for security reasons). * it makes it basically impossible to detect overruns on early small allocations with tools like valgrind, address sanitizer or friends as those allocations never hit malloc(), so are never traced by any custom tools. Any overrun will just go into the next variable, untracable. * allocators do their own buffering, alignment, and so on. If they are broken, your system is severely screwed. And if we need more money^H^H^H^Hemory we will hit that buggy allocatator anyway, making things harder to detect as it will only happen on "heavy" memory usage. This is not TruOS, SunOS 1.x or something like that anymore. We expect a reasonably sane OS. Everything else will get us screwed in much more severe ways. * it has shown issues when coming close to 4GiB allocations (#37, #109, older reports). DJB correctly says that an allocation of that size in your mailer shows that you have already done something wrong, but still: 64 bit platforms are the default, so using a 32 bit type to pass allocation sizes is a bug of it's own. * why bother at all with having a custom allocator?
DerDakon
added a commit
that referenced
this pull request
May 8, 2020
* it's as simple as possible, probably intentional. This makes things like alloc_re() (i.e. realloc()) inefficient as it does not look if the thing it reallocates is already the last thing in the batch, and simply extends that, or if alloc_free() has the last block and adds things back to the pool (ideally with zeroing for security reasons). * it makes it basically impossible to detect overruns on early small allocations with tools like valgrind, address sanitizer or friends as those allocations never hit malloc(), so are never traced by any custom tools. Any overrun will just go into the next variable, untracable. * allocators do their own buffering, alignment, and so on. If they are broken, your system is severely screwed. And if we need more money^H^H^H^Hemory we will hit that buggy allocatator anyway, making things harder to detect as it will only happen on "heavy" memory usage. This is not TruOS, SunOS 1.x or something like that anymore. We expect a reasonably sane OS. Everything else will get us screwed in much more severe ways. * it has shown issues when coming close to 4GiB allocations (#37, #109, older reports). DJB correctly says that an allocation of that size in your mailer shows that you have already done something wrong, but still: 64 bit platforms are the default, so using a 32 bit type to pass allocation sizes is a bug of it's own. * why bother at all with having a custom allocator?
DerDakon
added a commit
that referenced
this pull request
May 10, 2020
* it's as simple as possible, probably intentional. This makes things like alloc_re() (i.e. realloc()) inefficient as it does not look if the thing it reallocates is already the last thing in the batch, and simply extends that, or if alloc_free() has the last block and adds things back to the pool (ideally with zeroing for security reasons). * it makes it basically impossible to detect overruns on early small allocations with tools like valgrind, address sanitizer or friends as those allocations never hit malloc(), so are never traced by any custom tools. Any overrun will just go into the next variable, untracable. * allocators do their own buffering, alignment, and so on. If they are broken, your system is severely screwed. And if we need more money^H^H^H^Hemory we will hit that buggy allocatator anyway, making things harder to detect as it will only happen on "heavy" memory usage. This is not TruOS, SunOS 1.x or something like that anymore. We expect a reasonably sane OS. Everything else will get us screwed in much more severe ways. * it has shown issues when coming close to 4GiB allocations (#37, #109, older reports). DJB correctly says that an allocation of that size in your mailer shows that you have already done something wrong, but still: 64 bit platforms are the default, so using a 32 bit type to pass allocation sizes is a bug of it's own. * why bother at all with having a custom allocator?
DerDakon
added a commit
that referenced
this pull request
May 11, 2020
* it's as simple as possible, probably intentional. This makes things like alloc_re() (i.e. realloc()) inefficient as it does not look if the thing it reallocates is already the last thing in the batch, and simply extends that, or if alloc_free() has the last block and adds things back to the pool (ideally with zeroing for security reasons). * it makes it basically impossible to detect overruns on early small allocations with tools like valgrind, address sanitizer or friends as those allocations never hit malloc(), so are never traced by any custom tools. Any overrun will just go into the next variable, untracable. * allocators do their own buffering, alignment, and so on. If they are broken, your system is severely screwed. And if we need more money^H^H^H^Hemory we will hit that buggy allocator anyway, making things harder to detect as it will only happen on "heavy" memory usage. This is not TruOS, SunOS 1.x or something like that anymore. We expect a reasonably sane OS. Everything else will get us screwed in much more severe ways. * it has shown issues when coming close to 4GiB allocations (#37, #109, older reports). DJB correctly says that an allocation of that size in your mailer shows that you have already done something wrong, but still: 64 bit platforms are the default, so using a 32 bit type to pass allocation sizes is a bug of it's own. * why bother at all with having a custom allocator?
DerDakon
added a commit
that referenced
this pull request
May 11, 2020
* it's as simple as possible, probably intentional. This makes things like alloc_re() (i.e. realloc()) inefficient as it does not look if the thing it reallocates is already the last thing in the batch, and simply extends that, or if alloc_free() has the last block and adds things back to the pool (ideally with zeroing for security reasons). * it makes it basically impossible to detect overruns on early small allocations with tools like valgrind, address sanitizer or friends as those allocations never hit malloc(), so are never traced by any custom tools. Any overrun will just go into the next variable, untracable. * allocators do their own buffering, alignment, and so on. If they are broken, your system is severely screwed. And if we need more money^H^H^H^Hemory we will hit that buggy allocator anyway, making things harder to detect as it will only happen on "heavy" memory usage. This is not TruOS, SunOS 1.x or something like that anymore. We expect a reasonably sane OS. Everything else will get us screwed in much more severe ways. * it has shown issues when coming close to 4GiB allocations (#37, #109, older reports). DJB correctly says that an allocation of that size in your mailer shows that you have already done something wrong, but still: 64 bit platforms are the default, so using a 32 bit type to pass allocation sizes is a bug of it's own. * when any additional patch accidentially mixes calls to this functions with normal realloc()/free() this can lead to random crashes * why bother at all with having a custom allocator?
DerDakon
added a commit
that referenced
this pull request
May 12, 2020
* it's as simple as possible, probably intentional. This makes things like alloc_re() (i.e. realloc()) inefficient as it does not look if the thing it reallocates is already the last thing in the batch, and simply extends that, or if alloc_free() has the last block and adds things back to the pool (ideally with zeroing for security reasons). * it makes it basically impossible to detect overruns on early small allocations with tools like valgrind, address sanitizer or friends as those allocations never hit malloc(), so are never traced by any custom tools. Any overrun will just go into the next variable, untracable. * allocators do their own buffering, alignment, and so on. If they are broken, your system is severely screwed. And if we need more money^H^H^H^Hemory we will hit that buggy allocator anyway, making things harder to detect as it will only happen on "heavy" memory usage. This is not TruOS, SunOS 1.x or something like that anymore. We expect a reasonably sane OS. Everything else will get us screwed in much more severe ways. * it has shown issues when coming close to 4GiB allocations (#37, #109, older reports). DJB correctly says that an allocation of that size in your mailer shows that you have already done something wrong, but still: 64 bit platforms are the default, so using a 32 bit type to pass allocation sizes is a bug of it's own. * when any additional patch accidentially mixes calls to this functions with normal realloc()/free() this can lead to random crashes * why bother at all with having a custom allocator?
DerDakon
added a commit
that referenced
this pull request
May 12, 2020
* it's as simple as possible, probably intentional. This makes things like alloc_re() (i.e. realloc()) inefficient as it does not look if the thing it reallocates is already the last thing in the batch, and simply extends that, or if alloc_free() has the last block and adds things back to the pool (ideally with zeroing for security reasons). * it makes it basically impossible to detect overruns on early small allocations with tools like valgrind, address sanitizer or friends as those allocations never hit malloc(), so are never traced by any custom tools. Any overrun will just go into the next variable, untracable. * allocators do their own buffering, alignment, and so on. If they are broken, your system is severely screwed. And if we need more money^H^H^H^Hemory we will hit that buggy allocator anyway, making things harder to detect as it will only happen on "heavy" memory usage. This is not TruOS, SunOS 1.x or something like that anymore. We expect a reasonably sane OS. Everything else will get us screwed in much more severe ways. * it has shown issues when coming close to 4GiB allocations (#37, #109, older reports). DJB correctly says that an allocation of that size in your mailer shows that you have already done something wrong, but still: 64 bit platforms are the default, so using a 32 bit type to pass allocation sizes is a bug of it's own. * when any additional patch accidentially mixes calls to this functions with normal realloc()/free() this can lead to random crashes * why bother at all with having a custom allocator?
DerDakon
added a commit
that referenced
this pull request
May 20, 2020
* it's as simple as possible, probably intentional. This makes things like alloc_re() (i.e. realloc()) inefficient as it does not look if the thing it reallocates is already the last thing in the batch, and simply extends that, or if alloc_free() has the last block and adds things back to the pool (ideally with zeroing for security reasons). * it makes it basically impossible to detect overruns on early small allocations with tools like valgrind, address sanitizer or friends as those allocations never hit malloc(), so are never traced by any custom tools. Any overrun will just go into the next variable, untracable. * allocators do their own buffering, alignment, and so on. If they are broken, your system is severely screwed. And if we need more money^H^H^H^Hemory we will hit that buggy allocator anyway, making things harder to detect as it will only happen on "heavy" memory usage. This is not TruOS, SunOS 1.x or something like that anymore. We expect a reasonably sane OS. Everything else will get us screwed in much more severe ways. * it has shown issues when coming close to 4GiB allocations (#37, #109, older reports). DJB correctly says that an allocation of that size in your mailer shows that you have already done something wrong, but still: 64 bit platforms are the default, so using a 32 bit type to pass allocation sizes is a bug of it's own. * when any additional patch accidentially mixes calls to this functions with normal realloc()/free() this can lead to random crashes * why bother at all with having a custom allocator?
DerDakon
added a commit
that referenced
this pull request
May 22, 2020
* it's as simple as possible, probably intentional. This makes things like alloc_re() (i.e. realloc()) inefficient as it does not look if the thing it reallocates is already the last thing in the batch, and simply extends that, or if alloc_free() has the last block and adds things back to the pool (ideally with zeroing for security reasons). * it makes it basically impossible to detect overruns on early small allocations with tools like valgrind, address sanitizer or friends as those allocations never hit malloc(), so are never traced by any custom tools. Any overrun will just go into the next variable, untracable. * allocators do their own buffering, alignment, and so on. If they are broken, your system is severely screwed. And if we need more money^H^H^H^Hemory we will hit that buggy allocator anyway, making things harder to detect as it will only happen on "heavy" memory usage. This is not TruOS, SunOS 1.x or something like that anymore. We expect a reasonably sane OS. Everything else will get us screwed in much more severe ways. * it has shown issues when coming close to 4GiB allocations (#37, #109, older reports). DJB correctly says that an allocation of that size in your mailer shows that you have already done something wrong, but still: 64 bit platforms are the default, so using a 32 bit type to pass allocation sizes is a bug of it's own. * when any additional patch accidentially mixes calls to this functions with normal realloc()/free() this can lead to random crashes * why bother at all with having a custom allocator?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If n is close to 2**32 then the alignment calculation could cause an integer overflow. Rule this out by checking if the allocation is smaller than the fixed buffer. It could be done by checking against the possible space, which is done later anyway, but checking against SPACE has the benefit that this is a constant.
Alternative implementation for #37.