Bug in GetTracebackFunction (lua_scripting.cpp)

Posted by Twisol on Wed 15 Sep 2010 02:46 AM — 7 posts, 18,984 views.

Twisol USA Wed 15 Sep 2010 02:46 AM #0

I believe I've found a bug in GetTracebackFunction(). If the debug table isn't a table, it attempts to pop two items from the stack instead of just one. This is bad behavior, because it's messing with the part of the stack that calling code owns.

This is the problem code:

void GetTracebackFunction (lua_State *L)
  {
  // L: ...
  lua_pushliteral (L, LUA_DBLIBNAME);     // "debug"
  // L: ... "debug"
  lua_rawget      (L, LUA_GLOBALSINDEX);    // get debug library
  // L: ... debug_table

  if (!lua_istable (L, -1))
    {
    lua_pop (L, 2);   // pop result and debug table
    lua_pushnil (L);
    return;
    }

  // get debug.traceback
  lua_pushstring(L, "traceback");  
  lua_rawget    (L, -2);               // get getinfo function
  
  if (!lua_isfunction (L, -1))
    {
    lua_pop (L, 2);   // pop result and debug table
    lua_pushnil (L);
    return;
    }

  lua_remove (L, -2);   // remove debug table, leave traceback function
  }

I inserted lua stack-tracking comments to illustrate what's going wrong, too.

You could fix it by passing 1 instead of 2 to lua_pop(), but I decided to clean it up instead. Here's my proposed replacement:

void GetTracebackFunction (lua_State *L)
  {
  // L: ...
  lua_getfield (L, LUA_GLOBALSINDEX, LUA_DBLIBNAME);
  // L: ... debug

  if (lua_istable (L, -1))
    {
    lua_getfield (L, -1, "traceback");
    // L: ... debug, traceback
    lua_remove (L, -2);
    // L: ... traceback

    if (lua_isfunction (L, -1))
      return; // L: ... traceback
    }
  // L: ... unknown

  // can't find it; leave nil on the stack
  lua_pop (L, 1);
  lua_pushnil (L);
  return; // L: ... nil
  }

It's actually shorter, and it would be even more compact if I didn't have the trace comments there.

Twisol USA Wed 15 Sep 2010 03:06 AM #1

Confirmed:

Note(42) -- 42
debug = nil
Note(42) -- <error> attempt to call a nil value

Nick Gammon Australia Forum Administrator Wed 15 Sep 2010 04:25 AM #2

Looks like you are completely correct. Added in your amendments with slightly different comments. Thanks.

Twisol USA Wed 15 Sep 2010 04:28 AM #3

No problem. :)

Twisol USA Wed 15 Sep 2010 05:13 AM #4

May as well toss in my CallLuaWithTraceBack changes too. Nothing was broken and it works the same as before; I just wanted to logically separate the two steps of getting the traceback function and actually making the call.

int CallLuaWithTraceBack (lua_State *L, const int iArguments, const int iReturn)
  {
  // Set up the traceback function
  int iHandler = 0;
  GetTracebackFunction (L);
  if (!lua_isnil (L, -1))
    {
    // If we have the traceback function, put it in before the arguments and chunk
    iHandler = lua_gettop (L) - iArguments - 1;
    lua_insert (L, iHandler);
    }

  // Make the call
  int status = lua_pcall (L, iArguments, iReturn, iHandler);

  // Pop the traceback function if we had it.
  if (iHandler != 0)
    lua_remove (L, iHandler);

  return status;
  }  // end of CallLuaWithTraceBack

For a moment I thought perhaps someone could replace debug.traceback with something that could cause damage - i.e. not returning an error message and thus maybe throwing calling code off a stack index - but I can't seem to make anything happen beyond a lack of an error message. Oh well!

Nick Gammon Australia Forum Administrator Wed 15 Sep 2010 05:35 AM #5

Now this is wrong.

If GetTracebackFunction returns nil, then you have a nil on the stack which you didn't get rid of (and I did).

So if you do this:


debug = nil
Note (42)

You get this:


Run-time error
World: Smaug
Immediate execution
attempt to call a nil value

And this is why I don't want to just spend time "refactoring" with no advantage given other than "it looks nice".

The earlier case, yes you found a bug, and that was good to fix.

This one, I spent 10 minutes staring at your code thinking "what about the nil?". Then I checked the way that lua_pcall worked from the manual. Then I added in your change and made the test case I was doubtful about. Then I saw it was wrong. Now I type this.

So that's 30 minutes gone, just "making the code look nicer" (incorrectly as it turned out). Multiply that by hundreds of places you might find similar places to make it look nicer, and that is weeks of work.

Your assertion that "I can't seem to make anything happen ..." seems to suggest you didn't test the exact case your original post was about - the debug table not being present.

Twisol USA Wed 15 Sep 2010 05:48 AM #6

int CallLuaWithTraceBack (lua_State *L, const int iArguments, const int iReturn)
  {
  // Set up the traceback function
  int iHandler = 0;
  GetTracebackFunction (L);
  if (!lua_isnil (L, -1))
    {
    // If we have the traceback function, put it in before the arguments and chunk
    iHandler = lua_gettop (L) - iArguments - 1;
    lua_insert (L, iHandler);
    }
  else
    lua_pop (L, 1);

  // Make the call
  int status = lua_pcall (L, iArguments, iReturn, iHandler);

  // Pop the traceback function if we had it.
  if (iHandler != 0)
    lua_remove (L, iHandler);

  return status;
  }  // end of CallLuaWithTraceBack

I take your point. I didn't test, and that was wrong of me. I'm sorry I wasted your time when I could have found it without bringing you broken code.