Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression: MetaDataRef:set_string no longer no-op on nil value #14409

Closed
Montandalar opened this issue Feb 25, 2024 · 1 comment
Closed

Regression: MetaDataRef:set_string no longer no-op on nil value #14409

Montandalar opened this issue Feb 25, 2024 · 1 comment
Labels
Duplicate Unconfirmed bug Bug report that has not been confirmed to exist/be reproducible

Comments

@Montandalar
Copy link
Contributor

Montandalar commented Feb 25, 2024

Minetest version

Minetest 5.9.0-dev-b4be483d3 (Linux)
Using Irrlicht 1.9.0mt14
Using LuaJIT 2.1.0-beta3
Built by GCC 12.2
Running on Linux/6.1.0-18-amd64 x86_64
BUILD_TYPE=Debug
RUN_IN_PLACE=0
USE_CURL=1
USE_GETTEXT=1
USE_SOUND=1
STATIC_SHAREDIR="/usr/local/share/minetest"
STATIC_LOCALEDIR="/usr/local/share/locale"

Irrlicht device

X11

Operating system and version

Debian 12 bookworm

CPU model

AMD Ryzen 5 7600

GPU model

No response

Active renderer

No response

Summary

On trying to reprogram an ATC Controller node in Advtrains today I received the following error:

AsyncErr: Lua: Runtime error from mod '' in callback node_on_receive_fields(): /home/blockhead/.minetest/mods/advtrains/advtrains/atc.lua:146: bad argument #2 to 'set_string' (string expected, got nil)
stack traceback:
	[C]: in function 'set_string'
	/home/blockhead/.minetest/mods/advtrains/advtrains/atc.lua:146: in function </home/blockhead/.minetest/mods/advtrains/advtrains/atc.lua:123>

Previously the mod had always worked fine when updating ATC controllers. However, there are definitely some old vestiges in the controller code that were causing the problem:

                    --meta:set_string("mode", idxtrans[fields.mode])
                    meta:set_string("command", fields.command)
                    --meta:set_string("command_on", fields.command_on)
                    meta:set_string("channel", fields.channel)
                    --if fields.mode=="digiline" then
                    --  meta:set_string("infotext", attrans("ATC controller, mode @1\nChannel: @2", fields.mode, meta:get_string("command")) )

There is a lot of commented-out code related to an old idea of digilines connections to the tracks. There is also the obsolete field channel, which is not on the present formspec, but is used anyway to set the value of a metadata field on the node. So all requests to set it are actually just asking to set it to nil. That may just have been an error on the part of Advtrains, but it worked until now.

By git bisecting I found the pull request that introduced the problem: #14368. The set_string method was changed to construct a string_view with readParam:

@@ -115,9 +115,7 @@ int MetaDataRef::l_set_string(lua_State *L)
 
        MetaDataRef *ref = checkAnyMetadata(L, 1);
        std::string name = luaL_checkstring(L, 2);
-       size_t len = 0;
-       const char *s = lua_tolstring(L, 3, &len);
-       std::string str(s, len);
+       auto str = readParam<std::string_view>(L, 3);

which uses this template:

template <>
std::string_view LuaHelper::readParam(lua_State *L, int index)
{   
    size_t length;
    const char *str = luaL_checklstring(L, index, &length);
    return std::string_view(str, length);
}

which doesn't allow a nil string, and this check pushes an error onto the Lua stack and causes the first crash.

Solutions

  1. Make an exception to the nil checking applied in other parts of the code to allow the set_string function to continue to allow nil values and preserve backwards-compatibility.
  2. Document that the value argument must not be nil.

Steps to reproduce

  1. Install Advtrains and enable all the modules in a world.
  2. Place an ATC Controller node.
  3. Open the ATC Controller node's formspec with right-click.
  4. Click the Save button on the formspec.
@Montandalar Montandalar added the Unconfirmed bug Bug report that has not been confirmed to exist/be reproducible label Feb 25, 2024
@rollerozxa
Copy link
Member

rollerozxa commented Feb 25, 2024

Duplicate of #14391. Should be fixed by #14396.

@grorp grorp closed this as not planned Won't fix, can't repro, duplicate, stale Feb 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate Unconfirmed bug Bug report that has not been confirmed to exist/be reproducible
Projects
None yet
Development

No branches or pull requests

3 participants