Catch overflows in Gasman #2160
Conversation
ChrisJefferson
force-pushed
the
memmove-print
branch
from
7b9b57a
to
fd3621e
Compare
src/gasman.c
Outdated
| @@ -1325,7 +1339,8 @@ UInt ResizeBag ( | |||
| /* check that enough storage for the new bag is available */ | |||
| if ( SizeAllocationArea < WORDS_BAG(sizeof(BagHeader)+new_size) | |||
| && CollectBags( new_size, 0 ) == 0 ) { | |||
| return 0; | |||
| fputs("gap: cannot extend the workspace any more!!!!!!\n",stderr); | |||
| SyExit( 1 ); | |||
There was a problem hiding this comment.
Perhaps use SyAbortBags (not terribly important, just sayin')
There was a problem hiding this comment.
Ah, I didn't notice that function.
src/gasman.c
Outdated
| @@ -1943,6 +1960,11 @@ UInt CollectBags ( | |||
|
|
|||
| /* * * * * * * * * * * * * * * check phase * * * * * * * * * * * * * * */ | |||
|
|
|||
| // Check if this allocation would even fit into memory | |||
| if(((size_t)-1) - (size_t)(sizeof(BagHeader)+size) < (size_t)AllocBags) { | |||
There was a problem hiding this comment.
missing space after if. Also ((size_t)-1) should be SIZE_MAX (from ISO C).
But perhaps the more idiomatic way would be to add a check after setting stopBags:
// check for an overflow
if (stopBags < AllocBags)
SyAbortBags("gap: cannot extend the workspace any more!!");(I assume you want to abort, not return 0?
There was a problem hiding this comment.
Pointer overflow isn't defined. gcc will (for example) optimise i < i + 1 away to true for pointer i
src/gasman.c
Outdated
| && CollectBags( new_size-old_size, 0 ) == 0 ) { | ||
| return 0; | ||
| fputs("gap: cannot extend the workspace any more!!!!!\n",stderr); |
There was a problem hiding this comment.
So it seems the number of exclamation marks is supposed to help distinguish these errors?
There was a problem hiding this comment.
The seems to be the existing pattern, I just carried it on :)
ChrisJefferson
force-pushed
the
memmove-print
branch
from
fd3621e
to
a41e5fe
Compare
|
Hopefully all issues fixed up |
Catch a couple of cases where we can cause pointer overflow. Also no callers of NewBag or ResizeBag check the return value, so instead of returning 0 on failure exit GAP.
StopBags may be smaller or larger than EndBags, so we cannot use SpaceBetweenPointers. We switch to just using '-'.
ChrisJefferson
force-pushed
the
memmove-print
branch
from
a41e5fe
to
c0bb87f
Compare
Codecov Report
@@ Coverage Diff @@
## master #2160 +/- ##
==========================================
+ Coverage 69.58% 69.58% +<.01%
==========================================
Files 482 482
Lines 254628 254639 +11
==========================================
+ Hits 177182 177196 +14
+ Misses 77446 77443 -3
|
fingolfin
added
kind: bug
|
Do we want to backport this (and the |
fingolfin
added
release notes: added
The main purpose of this patch is to catch a couple of pointer overflows in gasman. I also add some tests to make future problems easier to catch.
I suspect there may be other overflows, particularly is the memory space gets very close to 4GB, but this fixes some problems I know about, and my prefered solution to fixing other problems would be instead to limit the size of the memory space, so (for example) the GAP workspace stops at least 1GB from the end of the memory space, and we limit largest object to 1GB.