Blockmap Size Error Checking (Fixes bug #1287) #1289
Conversation
| @@ -531,6 +531,10 @@ void P_LoadBlockMap (int lump) | |||
|
|
|||
| lumplen = W_LumpLength(lump); | |||
| count = lumplen / 2; | |||
| if(count >= 32766) | |||
There was a problem hiding this comment.
Why exactly this number?
There was a problem hiding this comment.
It's the blockmap limit for doom / 2 (count = blockmap size / 2, and blockmap limit is 65536 in doom, so I put that /2 and checked it against count.
There was a problem hiding this comment.
Source: https://doomwiki.org/wiki/Blockmap#Compression
and C data type size for short (which is what blockmap is) is 65536
There was a problem hiding this comment.
Thank you, but this was a rhetorical question. 😉 I'd prefer if magic numbers in source code didn't "fall from the sky". Something like USHRT_MAX/2 and an accompanying comment would probably be more meaningful.
There was a problem hiding this comment.
Also, 32766 is the wrong number since 65536 / 2 = 32768. A good example of why Fabian's approach is the better one.
How so? Demos seem to work fine when I'm running them. |
|
Well, refusing to load a map that played back before qualifies as breakage, I guess. 😉 |
|
Is there another way to check if the blockmap will crash? It seems it should be possible to make a way so that any list starting points past the 65536th byte won't load, but everything else will, so that it doesn't break compatibility. I'm not so sure about the internals of that, so I don't know how it would be implemented in the source code. |
| if(count >= 32766) | ||
| { | ||
| I_Error("Blockmap too large"); | ||
| } |
There was a problem hiding this comment.
Respect the code style. For example, put a space between if and (. Also, don't use tabs for indentation.
Code style: https://github.com/chocolate-doom/chocolate-doom/blob/master/HACKING.md
| @@ -531,6 +531,10 @@ void P_LoadBlockMap (int lump) | |||
|
|
|||
| lumplen = W_LumpLength(lump); | |||
| count = lumplen / 2; | |||
| if(count >= 32766) | |||
| { | |||
| I_Error("Blockmap too large"); | |||
There was a problem hiding this comment.
This is not a good error message. A better error message would include the two values that were compared and a link to the doomwiki.org page about the static limit.
| @@ -531,6 +531,10 @@ void P_LoadBlockMap (int lump) | |||
|
|
|||
| lumplen = W_LumpLength(lump); | |||
| count = lumplen / 2; | |||
| if(count >= 32766) | |||
There was a problem hiding this comment.
Also, 32766 is the wrong number since 65536 / 2 = 32768. A good example of why Fabian's approach is the better one.
| if(count >= 32766) | ||
| { | ||
| I_Error("Blockmap too large"); | ||
| } |
This checks for a large blockmap size and produces an error instead of allowing the map to mess up memory and cause a segfault.