Home page logo

nmap-dev logo Nmap Development mailing list archives

Re: [NSE] Resource Cleanup Upon Thread Death
From: Patrick Donnelly <batrick () batbytes com>
Date: Tue, 2 Jun 2009 17:37:08 -0600

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_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;
@@ -235,6 +238,10 @@
       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;

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'

-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

Sent through the nmap-dev mailing list
Archived at http://SecLists.Org

  By Date           By Thread  

Current thread:
[ Nmap | Sec Tools | Mailing Lists | Site News | About/Contact | Advertising | Privacy ]