Nmap Development mailing list archives
Re: [NSE] Better Handling for Require Errors
From: Patrick Donnelly <batrick () batbytes com>
Date: Sat, 11 Jun 2011 21:58:20 -0400
On Thu, Jun 9, 2011 at 7:00 PM, Fyodor <fyodor () insecure org> wrote:
On Wed, Jun 08, 2011 at 01:11:53PM -0400, Patrick Donnelly wrote:Running with -d shows a stack trace and makes it more obvious what the problem is ("module 'ipops' not found"), but can we do anything about this?Would artificially upping the debug level be acceptable when updating the database?Since I'm not 100% sure I understand all the details, I'm going to back up for a moment and note the initial problem... There were scripts which required certain libraries or privileges which were failing when run without those. There were two main offenders: scripts which would "require "openssl"' even though some people compile Nmap without that library, and scripts which required root privileges. The initial fix for this was a case-by-case system where we basically had a pcall template for each of those which we would paste into scripts where necessary. They looked like: if pcall(require,"openssl") then require("tns") else portrule = function() return false end action = function() end stdnse.print_debug( 3, "Skipping %s script because OpenSSL is missing.", SCRIPT_NAME) return; end and: if not nmap.is_privileged() then if not nmap.registry['firewalk'] then nmap.registry['firewalk'] = {} end if nmap.registry['firewalk']['rootfail'] then return false end nmap.registry['firewalk']['rootfail'] = true if nmap.verbosity() > 0 then nmap.log_write("stdout", SCRIPT_NAME .. ": not running for lack of privileges") end return false end Your (Patrick) patch eliminated these bloated sections by (if I understand correctly) allowing scripts to silently cease running if a require fails rather than printing a big backtrace and existing Nmap. You also root a 'root' library people could 'require' if they need root access. This reduced the blobs above to just: require "openssl" and require 'root' That looks a lot better!
Your analysis and understanding is correct.
But the issue is that this silent failure behavior can mask actual errors. This discussion is about a case where someone miscapitalized ipOps.lua. That caused script-updatedb to fail with a strange and unhelpful error message. The errors caused from trying to run such scripts directly is the same. When I change 'ipOps' to 'ipops' in http-tital.nse, I get: $ ./nmap -p80 --script scripts/http-title nmap.org Starting Nmap 5.52.IPv6.Beta2 ( http://nmap.org ) at 2011-06-09 15:52 PDT NSE: failed to initialize the script engine: /home/fyodor/nmap/nse_main.lua:603: attempt to index local 'script' (a nil value) stack traceback: /home/fyodor/nmap/nse_main.lua:603: in function 'get_chosen_scripts' /home/fyodor/nmap/nse_main.lua:1030: in main chunk [C]: ? QUITTING!
This is actually a "bug". Script.new was designed to return nil in the case of a silent error (whatever that may be; turns out the "silent require" patch was the first use-case for that). I fixed this bug in r23911.
I'm not sure what the error looked like before the silent require change. It might have been just as vague. In both cases (--script-udpatedb and trying to run the broken script), I get the more informative error message (noting the failure to find ipops.lua) in -d mode. So if the solution is increasing debug level, we should probably do so in general for this failed to initialize the script engine error rather than for --script-updatedb specifically.
Well, if we up the debug level everywhere then we have the error message all the time (even when we wanted it "silent"). The reason script writers used pcall was to avoid printing error output to users that would get confused (did the script break?). The goal of this patch was to make that unnecessary and to obsolete the hack being used to get around not being able to run (empty action/hostrule). As I see it we can do one of three things: (a) Go back to the old system. (b) Always print an error message. (c) Accept that require may be silenced for some border cases where we don't want it silenced. Script authors should be testing with -d... I'm partial to (c). I don't really like (a). I think (b) defeats the entire point. I'm open to suggestions... -- - Patrick Donnelly _______________________________________________ Sent through the nmap-dev mailing list http://cgi.insecure.org/mailman/listinfo/nmap-dev Archived at http://seclists.org/nmap-dev/
Current thread:
- Re: [NSE] Better Handling for Require Errors Patrick Donnelly (May 04)
- Re: [NSE] Better Handling for Require Errors David Fifield (Jun 07)
- Re: [NSE] Better Handling for Require Errors Patrick Donnelly (Jun 08)
- Re: [NSE] Better Handling for Require Errors Fyodor (Jun 09)
- Re: [NSE] Better Handling for Require Errors Patrick Donnelly (Jun 11)
- Re: [NSE] Better Handling for Require Errors Patrick Donnelly (Jun 11)
- Re: [NSE] Better Handling for Require Errors Patrik Karlsson (Jun 12)
- Re: [NSE] Better Handling for Require Errors Fyodor (Jun 12)
- Re: [NSE] Better Handling for Require Errors Patrick Donnelly (Jun 13)
- Re: [NSE] Better Handling for Require Errors Patrick Donnelly (Jun 08)
- Re: [NSE] Better Handling for Require Errors David Fifield (Jun 07)