Re: [NSE] nmap library problems

[David Fifield <david () bamsoftware com>]

On Mon, Dec 29, 2008 at 03:25:49PM -0700, Patrick Donnelly wrote:
> > > 6) I moved l_clock_ms from nse_nsock.cc (where it was oddly placed)
> > > over to nse_nmaplib.cc
> >
> > Your new code is correct, more concise and understandable, but its
> > behavior has changed in an significant way: it no longer returns
> > only integers.
> >
> > |_ ms: 1230526530786.8
> > |_ ms: 1230526536528
> > |_ ms: 1230526543972.5
> >
> > This may be good or bad, but have you tested it with existing scripts?
> > It only appears to be used in snmp.lua. From a perusal of the code it
> > looks like there is no problem because snmp rounds to an int itself, but
> > I would like you (or anyone) to test it.
>
> I changed it to return the ceiling of the result.

That works for me. Does anyone have a use for a higher resolution timer
in NSE? We could get up to nanosecond resolution potentially, though I
think a better interface for such a timer would give results in seconds
rather than nanoseconds.

> > You changed existing behavior in a significant way in newtry_finalize
> > (formerly l_exc_finalize) too. l_exc_finalize handled an exception on
> > false or nil, continued successfully on true (and only true), and quit
> > on any other value. newtry_finalize treats any value that is not false
> > or nil as true. I can see this causing problems for a programmer new to
> > newtry, especially as is is different from Lua's assert in the true
> > case. Someone might wrongly return just a host name and a port number on
> > success, say, instead of returning true first. With l_exc_finalize Nmap
> > would crash with an error message (an easy bug). With the new
> > implementation the script's host name would be a number and the port
> > number would be nil (a harder bug).
>
> I changed it to print an error message if the first value is not a
> boolean. Having the script cause Nmap to exit is too strict in my
> opinion.

You're right, aborting the script rather than killing Nmap is better.
Just checking that the first argument is a boolean is not quite right,
because lots of library functions raise an exception by returning nil
not false. The logic should be

nil or false: indicates an exception.
true: indicates success.
anything else: "finalizing a non-conforming function".

At least until we enforce a standard on returning false or nil (if we
ever do).

Good work on this. Anyone's who has worked in nse_nmaplib.cc will
appreciate the improvements. (If you haven't you'll have to take my word
for it.) Especially the set_port_state/set_port_version functions are a
lot better.

David Fifield

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