Re: Janitorial patch re: strtol

[William Pursell <bill.pursell () gmail com>]

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.  'make check'
works in ncat...but there doesn't seem to be any test suite for
nmap itself.  Is that right?

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.


diff --git a/TargetGroup.cc b/TargetGroup.cc
index fdbb6e1..8c91290 100644
--- a/TargetGroup.cc
+++ b/TargetGroup.cc
@@ -187,17 +187,12 @@ int TargetGroup::parse_expr(const char * const target_expr, int af) {

       *s = '\0';  /* Make sure target_net is terminated before the /## */
       s++;        /* Point s at the netmask */
-      if (!isdigit((int) (unsigned char) *s)) {
+      netmask_long = parse_long(s, (char**) &tail);
+      if (*tail != '\0' || tail == s || netmask_long < 0 || netmask_long > 32) {
         error("Illegal netmask value, must be /0 - /32 .  Assuming /32 (one host)");
-        netmask = 32;
-      } else {
-        netmask_long = strtol(s, (char**) &tail, 10);
-        if (*tail != '\0' || tail == s || netmask_long < 0 || netmask_long > 32) {
-          error("Illegal netmask value, must be /0 - /32 .  Assuming /32 (one host)");
           netmask = 32;
-        } else
+      } else
           netmask = (u32) netmask_long;
-      }
     } else
       netmask = 32;
     resolvedname = hostexp;
diff --git a/nbase/nbase.h b/nbase/nbase.h
index 11f2cb1..8882000 100644
--- a/nbase/nbase.h
+++ b/nbase/nbase.h
@@ -390,6 +390,10 @@ int Snprintf(char *, size_t, const char *, ...)
    length strlength are printable (as defined by isprint()) */
 int stringisprintable(const char *str, int strlength);

+/* parse_long is like strtol or atoi, but it allows digits only.
+   No whitespace, sign, or radix prefix. */
+long parse_long(const char *s, char **tail);
+
 /* Convert non-printable characters to replchar in the string */
 void replacenonprintable(char *str, int strlength, char replchar);

diff --git a/nbase/nbase_misc.c b/nbase/nbase_misc.c
index 6a680bb..94da1cb 100644
--- a/nbase/nbase_misc.c
+++ b/nbase/nbase_misc.c
@@ -656,4 +656,16 @@ char *hexdump(const u8 *cp, u32 length){
   return buffer;
 } /* End of hexdump() */

+/* This is like strtol or atoi, but it allows digits only. No whitespace, sign,
+   or radix prefix. */
+long parse_long(const char *s, char **tail)
+{
+    if (!isdigit((int) (unsigned char) *s)) {
+        *tail = (char *) s;
+        return 0;
+    }
+
+    return strtol(s, (char **) tail, 10);
+}
+

diff --git a/ncat/util.c b/ncat/util.c
index 757d8e1..daecfbf 100644
--- a/ncat/util.c
+++ b/ncat/util.c
@@ -272,18 +272,6 @@ char *mkstr(const char *start, const char *end)
     return s;
 }

-/* This is like strtol or atoi, but it allows digits only. No whitespace, sign,
-   or radix prefix. */
-long parse_long(const char *s, char **tail)
-{
-    if (!isdigit((int) (unsigned char) *s)) {
-        *tail = (char *) s;
-        return 0;
-    }
-
-    return strtol(s, (char **) tail, 10);
-}
-
 /* Return true if the given address is a local one. */
 int addr_is_local(const union sockaddr_u *su)
 {
diff --git a/ncat/util.h b/ncat/util.h
index ac14e03..4da9b51 100644
--- a/ncat/util.h
+++ b/ncat/util.h
@@ -132,8 +132,6 @@ int strbuf_sprintf(char **buf, size_t *size, size_t *offset, const char *fmt, ..

 char *mkstr(const char *start, const char *end);

-long parse_long(const char *s, char **tail);
-
 int addr_is_local(const union sockaddr_u *su);

 const char *inet_socktop(const union sockaddr_u *su);

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