Re: Janitorial patch re: strtol

[David Fifield <david () bamsoftware com>]

On Sat, May 08, 2010 at 10:09:41AM -1000, William Pursell wrote:
> David Fifield wrote:
> > So I'm going to ask you to expand your patch somewhat, following these
> > steps.
> >     * Move parse_long from ncat/utils.c to nbase/nbase_misc.c.
> >     * Change the netmask parsing to use parse_long.
> > parse_long is used a lot in Ncat, particularly in the HTTP parser where
> > the protocol doesn't allow any non-digits in number in certain places.
> > It will probably be useful in other programs so nbase is a good place
> > for it. By the way, I was never thrilled with the name parse_long, so if
> > you have a better idea please suggest it.
>
> I think parse_long is actually a pretty good name.  I've included
> a patch that does the above, and have a few questions.

Thanks very much. This is committed in r17512.

> 'make check' works in ncat...but there doesn't seem to be any test
> suite for nmap itself.  Is that right?

That's right. For Ncat, "make check" only tests a few internal functions
(but it's a good test for parse_long because that function gets
exercised). There are more comprehensive external tests in
test/ncat-test.pl. Those aren't run by default because they're kind of
slow.

> I notice that every call to parse_long() clears errno first.  This
> seems like something that parse_long could do.  Also, if parse_long()
> detects non-digit, it seems reasonable to have it set errno=ERANGE.
> I considered doing that in this patch, but was a little concerned
> about edge cases in http.c where errno is checked but *tail is not,
> so I left it unchanged.

I don't know, I want parse_long to work pretty much the same way as
strtol, which doesn't clear errno. In general, libc functions don't do
that. Likewise, strtol doesn't set ERANGE when the first character is
not a valid digit or prefix, it just returns 0 with *tail == s.

Every call to parse_long should have a check for errno != 0 and
*tail == s. (It appears that they all do, in Ncat.) It's not always
appropriate to check **tail == '\0', because the number you're parsing
doesn't always come at the end of the string. So that's why that check
is sometimes omitted.

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