Re: Nmap uses ctype macros improperly

[J Marlow <joshmarlow () gmail com>]

On Mon, Jul 27, 2009 at 6:43 PM, David Fifield <david () bamsoftware com>wrote:

> On Sat, Jul 18, 2009 at 05:20:34PM +0400, Solar Designer wrote:
> > There are lots of improper uses of ctype macros, such as isupper() and
> > tolower() (and many others), in Nmap source code (latest in SVN).  In
> > fact, most of the uses look problematic.
> >
> > Some invoke the macros on "char" variables or expressions, which are
> > signed on most archs and with most compilers.  This breaks down when the
> > C library directly uses the macro's argument as array subscript and does
> > not provide a 384-element array (some do, specifically as a workaround
> > for improper uses like that).  Indeed, behavior on negative array
> > subscripts is undefined anyway, but those C libraries with the
> > workaround assume "support" by the "native" C compiler (or at least
> > provide the workaround in case this happens to be "supported").
>
> Thank you. I was not aware of this problem. I have asked Josh, one of
> our Summer of Code bug wranglers, to audit the source code and fix any
> problematic instances.
>
> > Thus, the best approach appears to be to cast any "char" expressions to
> > (int)(unsigned char) (yes, double-cast) before passing them to the ctype
> > macros.  In some contexts, code such as:
> >
> > int c;
> > ...
> > c = (unsigned char)*p++;
> > ...
> > if (islower(c)) ...;
>
> I understand your reason for suggesting (int) (unsigned char), but I
> asked Josh to use just (unsigned char). Not being able to index an array
> with an unsigned char seems like a compiler or C library bug, and we
> have to decide how far we're willing to work around things.


Hi all,

I've completed my audit of ctype calls in nmap, nbase, ncat and nsock and
have added explicit casts of the form (int)(unsigned char), where
appropriate, except cases where the argument was already an unsigned char.
These changes were in commits r14789 and r14790.  I've also updated the
CHANGELOG to reflect this.

Cheers,
Josh



> I would like to have a solution that doesn't require every call to have
> a cast, and is not affected by locale. I see that GNU has a safe-ctype.h
> include that is not affected by locale. I would also like a set of
> functions that only process unsigned char; The fact that the ctype.h
> functions must also process EOF is the only reason for them to take an
> int as far as I know, and it is not very useful. It's usually handled in
> a separate check, as in this code from ncat/ncat_hostmatch.c:
>
>    /* Skip whitespace. */
>    while ((c = getc(fd)) != EOF) {
>        if (!isspace(c))
>            break;
>    }
>
> Thanks for your observation Alexander. The above paragraph is just me
> thinking out loud.
>
> David Fifield
>
> _______________________________________________
> Sent through the nmap-dev mailing list
> http://cgi.insecure.org/mailman/listinfo/nmap-dev
> Archived at http://SecLists.Org

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