Fix cChunkMap issues below with coords below y=0 #5397
Conversation
|
Sorry for the delay in reviewing, I was a bit busy - I'll get round to it tomorrow. |
There was a problem hiding this comment.
This looks good generally. I think it might be slightly better though if we created new functions that do this checking, cWorld.SafeBlockTypeMeta or something - otherwise the behaviour of chunks' GBTM and worlds' GTBM functions will be different which could lead to confusion.
| @@ -585,6 +595,11 @@ void cChunkMap::SetBlock(Vector3i a_BlockPos, BLOCKTYPE a_BlockType, NIBBLETYPE | |||
|
|
|||
| bool cChunkMap::GetBlockTypeMeta(Vector3i a_BlockPos, BLOCKTYPE & a_BlockType, NIBBLETYPE & a_BlockMeta) const | |||
| { | |||
| if (!cChunkDef::IsValidHeight(a_BlockPos.y)) | |||
| { | |||
| return false; | |||
There was a problem hiding this comment.
I have a slight concern here that we might end up with use of uninitialised memory as a consequence of this return - idiomatic use of GBTM does not initialise the arguments, and with an early return they are not initialised here either.
I think we should zero them here for safety.
There was a problem hiding this comment.
I could be wrong, but I think there are other cases where early return does not initialize the arguments; for example, if !Chunk->IsValid(). I don't entirely understand when a Chunk would be invalid, but that's another case where the arguments will be left uninitialized. Also, most (all?) C++ compilers should zero the values anyway afaik, since they're enums, although I'm not totally sure.
There was a problem hiding this comment.
You're right, we need to add an initialisation to the invalid chunk case as well. Most C++ compilers do zero-initialise variables, but that is non-standard undefined behaviour which we should not rely on.
There was a problem hiding this comment.
I think the modus operandi "If the return value is false, then the out params are uninitialized" is valid. It should just probably be mentioned in the doxy-comment. All the callsites should be doing if (!GetBlockTypeMeta(...)) { return ...; } so the uninitialized values don't matter.
There was a problem hiding this comment.
Perhaps, is there some marker that would tell the compilers that ignoring the return value of this function is a bad thing?
Should fix #5395 and some other issues. See discussion in #5220. I noticed that lighting the void on fire (using a flint and steel on the side of bedrock at y=0) crashed the server before this PR, but not after. This should fix most other similar issues.