Gammon Forum : MUSHclient : Development : Bug in GetTracebackFunction (lua

Entire forum ➜ MUSHclient ➜ Development ➜ Bug in GetTracebackFunction (lua_scripting.cpp)

Bug in GetTracebackFunction (lua_scripting.cpp)

It is now over 60 days since the last post. This thread is closed. Refresh page

Posted by Twisol USA (2,257 posts) Bio

Date

Wed 15 Sep 2010 02:46 AM (UTC)

Amended on Wed 15 Sep 2010 02:49 AM (UTC) by Twisol

Message

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.

'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol

Top

Posted by Twisol USA (2,257 posts) Bio

Date

Reply #1 on Wed 15 Sep 2010 03:06 AM (UTC)

Amended on Wed 15 Sep 2010 03:11 AM (UTC) by Twisol

Message

Confirmed:

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

'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol

Top

Posted by

Nick Gammon Australia (23,163 posts) Bio Forum Administrator

Date

Reply #2 on Wed 15 Sep 2010 04:25 AM (UTC)

Message

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

- Nick Gammon

www.gammon.com.au, www.mushclient.com

Top

Posted by

Twisol USA (2,257 posts) Bio

Date

Reply #3 on Wed 15 Sep 2010 04:28 AM (UTC)

Message

No problem. :)

'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol

Top

Posted by Twisol USA (2,257 posts) Bio

Date

Reply #4 on Wed 15 Sep 2010 05:13 AM (UTC)

Amended on Wed 15 Sep 2010 05:17 AM (UTC) by Twisol

Message

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!

'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol

Top

Posted by Nick Gammon Australia (23,163 posts) Bio Forum Administrator

Date Reply #5 on Wed 15 Sep 2010 05:35 AM (UTC)

Message

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.

- Nick Gammon

www.gammon.com.au, www.mushclient.com

Top

Posted by Twisol USA (2,257 posts) Bio

Date

Reply #6 on Wed 15 Sep 2010 05:48 AM (UTC)

Amended on Wed 15 Sep 2010 05:52 AM (UTC) by Twisol

Message

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.

'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol

Top

The dates and times for posts above are shown in Universal Co-ordinated Time (UTC).

To show them in your local time you can join the forum, and then set the 'time correction' field in your profile to the number of hours difference between your location and UTC time.

15,742 views.

It is now over 60 days since the last post. This thread is closed. Refresh page

Go to topic: Search the forum

top