Re: [NSE] Resource Cleanup Upon Thread Death

[Patrick Donnelly <batrick () batbytes com>]

Hi David,

On Tue, Jun 2, 2009 at 4:41 PM, David Fifield <david () bamsoftware com> wrote:
> I think this patch is good and should be committed, but first I ahve
> just a few questions about the code.
>
> > Index: nse_nmaplib.cc
> > ===================================================================
> > --- nse_nmaplib.cc    (revision 13460)
> > +++ nse_nmaplib.cc    (working copy)
> > @@ -226,6 +226,9 @@
> >        {
> >          lua_pushthread(L);
> >          lua_replace(L, lua_upvalueindex(2)); // set running
> > +        lua_pushvalue(L, lua_upvalueindex(3)); // unique identifier
> > +        lua_pushvalue(L, lua_upvalueindex(4)); // aux_mutex_done closure
> > +        nse_destructor(L, 'a');
> >          return 0;
> >        }
> >        lua_pushthread(L);
> > @@ -235,6 +238,10 @@
> >        lua_pushthread(L);
> >        if (!lua_equal(L, -1, lua_upvalueindex(2)))
> >          luaL_error(L, "%s", "Do not have a lock on this mutex");
> > +      /* remove destructor */
> > +      lua_pushvalue(L, lua_upvalueindex(3));
> > +      nse_destructor(L, 'r');
> > +      /* set new thread to lock the mutex */
> >        lua_getfield(L, LUA_REGISTRYINDEX, "_LOADED");
> >        lua_getfield(L, -1, "table");
> >        lua_getfield(L, -1, "remove");
> > @@ -242,10 +249,14 @@
> >        lua_pushinteger(L, 1);
> >        lua_call(L, 2, 1);
> >        lua_replace(L, lua_upvalueindex(2));
> > -      if (!lua_isnil(L, lua_upvalueindex(2))) // waiting threads had a thread
> > +      if (lua_isthread(L, lua_upvalueindex(2))) // waiting threads had a thread
> >        {
> > -        assert(lua_isthread(L, lua_upvalueindex(2)));
> > -        nse_restore(lua_tothread(L, lua_upvalueindex(2)), 0);
> > +        lua_State *thread = lua_tothread(L, lua_upvalueindex(2));
> > +        lua_pushvalue(L, lua_upvalueindex(3)); // destructor key
> > +        lua_pushvalue(L, lua_upvalueindex(4)); // destructor
> > +        lua_xmove(L, thread, 2);
> > +        nse_destructor(thread, 'a');
> > +        nse_restore(thread, 0);
>
> I am having trouble keeping track of the upvalue indices associated with
> a thread. I would like you to add a big block comment describing what
> each index contains.

Alright, I've added that.

> > +  function Thread:close ()
> > +    local ch = self.close_handlers;
> > +    for i = #ch, 1, -1 do
> > +      ch[i].destructor(ch[i].thread, i);
> > +      ch[i] = nil;
> > +    end
> > +    for key, destructor_t in pairs(ch) do
> > +      destructor_t.destructor(destructor_t.thread, key);
> > +      ch[key] = nil;
> > +    end
> > +  end
>
> Please explain what this bit of code is doing and why there are two
> loops. It looks like you are calling all the integer-keyed destructors
> in reverse order, then all the destructors with other keys in whatever
> order they appear. What is the structure of close_handlers? Is it
> possible to enforce integer keys only and simply this?

This is a remnant of something else I was doing and I have removed the
integer key part. The structure is simply unique destructor keys to a
table containing two fields (you can see these tables made in the
definition of _R[DESTRUCTOR] in nse_main.lua).

> > @@ -396,6 +412,7 @@
> >    -- nse_restore, waiting threads become pending and later are moved all
> >    -- at once back to running.
> >    local running, waiting, pending = {}, {}, {}
> > +  local all = setmetatable({}, {__mode = "kv"}); -- base coroutine to Thread
> >    -- hosts maps a host to a list of threads for that host.
> >    local hosts, total = {}, 0
> >    local current
> > @@ -406,19 +423,37 @@
> >      local thread = remove(threads);
> >      thread:d("Starting %THREAD against %s%s.", thread.host.ip,
> >          thread.port and ":"..thread.port.number or "");
> > -    running[thread.co], total = thread, total + 1;
> > +    all[thread.co], running[thread.co], total = thread, thread, total+1;
> >      hosts[thread.host] = hosts[thread.host] or {};
> >      hosts[thread.host][thread.co] = true;
> >    end
>
> Is the new "all" table to facilitate your planned change to allow a
> script thread to start its own coroutines?

This all table simply maps the original thread (coroutine) to the
Thread Class it belongs to. I removed the changes (also remnants of
other things I was working on) to WAITING_TO_RUNNING, which probably
made it more confusing.. We need the all table to obtain the
coroutine's Thread (Class). The coroutine we are setting a destructor
on may not be running (if it were, we could use the 'current'
variable).

-- 
-Patrick Donnelly

"Let all men know thee, but no man know thee thoroughly: Men freely
ford that see the shallows."

- Benjamin Franklin

Attachment: resource_collection2.patch
Description:

_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://SecLists.Org