API: Calling functions taking positions with a negative / high y position crashes the server

[Seadragon91]

Server OS: Linux Cuberite Commit id: ce8d8388d630fa936500e6843496afe113ad8bc5

Actual behavior

Server crashes.

Steps to reproduce the behavior

Call this code from a plugin: cRoot:Get():GetDefaultWorld():GrowTreeFromSapling(Vector3d(0, 1000, 0))

The chunk has to be loaded, join the server and teleport to 0 1000 0.

Negative y position also crashes the server (Moved over from #5191):

cRoot:Get():GetDefaultWorld():DigBlock(1, -100, 1, nil)
cRoot:Get():GetDefaultWorld():IsBlockDirectlyWatered(1, -100, 1)
cRoot:Get():GetDefaultWorld():SetTrapdoorOpen(1, -100, 1, false)
cRoot:Get():GetDefaultWorld():QueueBlockForTick(1, -57, 1, 1)
cRoot:Get():GetDefaultWorld():IsTrapdoorOpen(1, -100, 1)

Stack trace

#0  0x000000000042a933 in ChunkDataStore<unsigned char, 2048ul, (unsigned char)0>::Get(Vector3<int>) const ()
#1  0x000000000042d22f in cChunkMap::GetBlockMeta(Vector3<int>) const ()
#2  0x00000000004ada5d in cWorld::GrowTreeFromSapling(Vector3<int>) ()
#3  0x00000000005349c2 in tolua_cWorld_GrowTreeFromSapling(lua_State*) ()
#4  0x0000000000796a5e in luaD_precall(lua_State*, lua_TValue*, int) ()
#5  0x00000000007a7b73 in luaV_execute(lua_State*, int) ()
#6  0x0000000000796ec9 in luaD_call(lua_State*, lua_TValue*, int) ()
#7  0x00000000007971d6 in luaD_pcall(lua_State*, void (*)(lua_State*, void*), void*, long, long) ()
#8  0x0000000000792e7e in lua_pcall ()
#9  0x000000000053c64e in cLuaState::CallFunction(int) ()
#10 0x000000000055ab16 in bool cLuaState::Call<cLuaState::cRef, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, cPlayer*&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, cLuaState::cRet const&, bool&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&>(cLuaState::cRef const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, cPlayer*&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, cLuaState::cRet const&, bool&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) ()
#11 0x000000000055aa2f in bool cLuaState::cCallback::Call<std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, cPlayer*&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, cLuaState::cRet const&, bool&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&>(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, cPlayer*&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, cLuaState::cRet const&, bool&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) ()
#12 0x000000000055a94e in LuaCommandHandler::ExecuteCommand(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, cPlayer*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, cCommandOutputCallback*) ()
#13 0x000000000058ca47 in cPluginManager::ExecuteConsoleCommand(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, cCommandOutputCallback&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ()
#14 0x0000000000491609 in cServer::ExecuteConsoleCommand(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, cCommandOutputCallback&) ()
#15 0x00000000004877bc in cRoot::QueueExecuteConsoleCommand(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, cCommandOutputCallback&) ()
#16 0x000000000048751a in cRoot::HandleInput() ()
#17 0x0000000000484bf3 in cRoot::Run(cSettingsRepositoryInterface&) ()
#18 0x00000000004b7cc0 in main ()

[12xx12]

I think this happens for any position related function.

I thought about having a check in the Vector3 constructor but this isn't user friendly in case someone is doing calculations. I think we have to manually add checks.

[KingCol13]

I'm moving my discussion here.

To summarise #5395, there are also crashes when:

• mining blocks from under bedrock while in survival mode
• Attempting to push blocks with a piston below down below y=0

These are both due to GetBlockTypeMeta being called

The discussion in #5194 suggests we should do these checks before calling GetBlockTypeMeta. The alternative would be adding a void block and an additional check in cChunkMap::GetBlockTypeMeta and if IsValidHeight is false, return false and set the void block.

I think it's best for the calling methods to check IsValidHeight to keep things clean (and maybe performant too, as although GetBlockTypeMeta isn't called much GetBlock and GetBlockMeta seem to be called very often).

[hle0]

Hi, @KingCol13, I'm interested in trying to fix this issue and the related issues (stuff happening below y=0). Are there still things to fix? It looks like #5194 fixes some of these issues, and was merged last year. Has there been any more recent progress on these types of issues I should know about?

[12xx12]

Hi @hle0

an easy way would be to change GetBlockTypeMeta in a way that it returns air if a non-valid position is passed. Maybe add a comment to the function to explain this behaviour

[12xx12]

And there is no progress. Usually the development takes place publicly in the PR.

[12xx12]

And GetBlock, GetMeta, GetBlockTypeMeta will get replaced with one method in the block state PR

[KingCol13]

Hi @hle0

an easy way would be to change GetBlockTypeMeta in a way that it returns air if a non-valid position is passed. Maybe add a comment to the function to explain this behaviour

This would work for a lot but there might be some cases (like pistons https://github.com/cuberite/cuberite/issues/5395#issuecomment-1075616950) where we don't want void blocks to behave like air blocks. Also we were considering not putting checks in those functions for performance: https://github.com/cuberite/cuberite/pull/5194#issuecomment-877851697. I think the long term solution would be a separate void block but it's probably fine until blockstate gets merged if we make a note.

Another intermediate solution could be more manual bindings.

[hle0]

an easy way would be to change GetBlockTypeMeta in a way that it returns air if a non-valid position is passed. Maybe add a comment to the function to explain this behaviour

Another thing we could do is return false if the height is invalid. cChunkMap::GetBlockTypeMeta already returns false if the chunk is invalid. Unfortunately, it looks like a lot of the code that indirectly calls cChunkMap::GetBlockTypeMeta doesn't pay attention to this return value. I think it would make more sense (be more maintainable, and maybe more performant) if we fixed those invocations so that they actually do check the return value.

Here's an example of code that doesn't pay attention to the return value:

bool cBlockPistonHandler::CanPushBlock(
    const Vector3i & a_BlockPos, cWorld & a_World, bool a_RequirePushable,
    Vector3iSet & a_BlocksPushed, const Vector3i & a_PushDir
)
{
    const static std::array<Vector3i, 6> pushingDirs =
    {
        {
            Vector3i(-1,  0,  0), Vector3i(1, 0, 0),
            Vector3i( 0, -1,  0), Vector3i(0, 1, 0),
            Vector3i( 0,  0, -1), Vector3i(0, 0, 1)
        }
    };

    BLOCKTYPE currBlock;
    NIBBLETYPE currMeta;
    a_World.GetBlockTypeMeta(a_BlockPos, currBlock, currMeta);

    if (!cChunkDef::IsValidHeight(a_BlockPos.y))
    {
        return !a_RequirePushable;
    }
        ...

[KingCol13]

Here's an example of code that doesn't pay attention to the return value:

Yea that's one I fixed in #5396, maybe it makes more sense to do it as you suggested.

[hle0]

Yea that's one I fixed in https://github.com/cuberite/cuberite/pull/5396, maybe it makes more sense to do it as you suggested.

I can work on adding checks to everything that needs it, and then I'll make it a PR. It looks like there are a lot of things; for example, if you try to light the void on fire (with a flint and steel on the side of a block at y=0), it'll crash because of GetBlock, not GetBlockTypeMeta, I think. So that whole family of functions should probably be fixed.