openssh unix dev - Jul 2009 - Non-standard conformant usage of ctype functions

Hi,

Per the definitions of the ctype functions in POSIX-1.2008 "the c
argument is an int, the value of which the application shall ensure is a
character representable as an unsigned char or equal to the value of the
macro EOF. If the argument has any other value, the behavior is
undefined."

For obvious reasons this results in problems if you use signed char
variables as parameters in calls to the ctype functions, if you support
other charsets than ASCII.
In theory OpenSSH has this problem as well.  With a few exceptions it
uses (signed) char values throughout in calls to ctype functions like
isspace.

However, given that OpenSSH doesn't call setlocale, the locale is always
"C" and the chance that the ctype functions return a wrong value is
rather
tiny.  Nevertheless, for the sake of correctness, and to avoid potential
problems in some later, locale-aware version of OpenSSH, I'd like to
propose to change all calls of ctype functions in OpenSSH from, for
instance:

  if (isupper(c))
    c = (char)tolower(c);

to:

  if (isupper((u_char)c))
    c = (char)tolower((u_char)c);

If that's ok with you, I'd create a matching patch.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Corinna Vinschen wrote:
> Hi,
> 
> Per the definitions of the ctype functions in POSIX-1.2008 "the c
> argument is an int, the value of which the application shall ensure is a
> character representable as an unsigned char or equal to the value of the
> macro EOF. If the argument has any other value, the behavior is
> undefined."
> 
> For obvious reasons this results in problems if you use signed char
> variables as parameters in calls to the ctype functions, if you support
> other charsets than ASCII.
> In theory OpenSSH has this problem as well.  With a few exceptions it
> uses (signed) char values throughout in calls to ctype functions like
> isspace.
No idea why is not reported.
> However, given that OpenSSH doesn't call setlocale, the locale is
always
> "C" and the chance that the ctype functions return a wrong value
is rather
> tiny.  Nevertheless, for the sake of correctness, and to avoid potential
> problems in some later, locale-aware version of OpenSSH, I'd like to
> propose to change all calls of ctype functions in OpenSSH from, for
> instance:
> 
>   if (isupper(c))
>     c = (char)tolower(c);
> 
> to:
> 
>   if (isupper((u_char)c))
>     c = (char)tolower((u_char)c);
I'm using in my X.509 certificate support patch following macro:

#ifndef ISSPACE
#  define ISSPACE(ch) (isspace((int)(unsigned char)(ch)))
#endif

to avoid problem with signed strings(characters). If I remember well 
affected platform is solaris 8(?).
I think that other functions like isalpha, isdigit are impacted too.

> If that's ok with you, I'd create a matching patch.
May be a patch based on macros like ISPACE above, but the question is 
why to patch code if it not reported a issue ?
> Corinna
> 
Roumen