additional compiler hardening flags

[Corinna Vinschen]

On Jan 17 10:10, Darren Tucker wrote:
> On Thu, Jan 16, 2014 at 11:44:26PM +0100, Corinna Vinschen wrote:
> > Will you accept patches to get rid of the existing warnings so we can
> > build all of OpenSSH with -Werror?
> 
> Probably yes as long as it doesn't break any currently-working
> platforms, and isn't a large maintenace burden.  At one point I could
> compile on Fedora warning-free including PAM support, but I see a couple
> of warnings have crept in due to either pickier compilers or platform
> changes.
> 
> So warning fixes for -portable specific code: sure.  For code shared
> with openbsd, yes if we can push it back upstream or do it in a way
> that's doesn't cause every diff we pull to be a hassle.  Other things
> maybe depending on what it is.
> 
> What have you got?  I'll open the bidding.
> 
> loginrec.c: In function 'login_get_lastlog':
> loginrec.c:316:7: warning: format '%lu' expects argument of type
> 'long unsigned int', but argument 3 has type 'size_t' [-Wformat]
> loginrec.c:316:7: warning: format '%lu' expects argument of type
> 'long unsigned int', but argument 4 has type 'unsigned int' [-Wformat]
> 
> Index: loginrec.c
> ===================================================================
> RCS file: /home/dtucker/openssh/cvs/openssh/loginrec.c,v
> retrieving revision 1.93
> diff -u -p -r1.93 loginrec.c
> --- loginrec.c	29 Dec 2013 06:40:19 -0000	1.93
> +++ loginrec.c	16 Jan 2014 22:58:07 -0000
> @@ -313,7 +313,8 @@ login_get_lastlog(struct logininfo *li, 
>  	if (strlcpy(li->username, pw->pw_name, sizeof(li->username)) >=
>  	    sizeof(li->username)) {
>  		error("%s: username too long (%lu > max %lu)", __func__,
> -		    strlen(pw->pw_name), sizeof(li->username) - 1);
> +		    (unsigned long)strlen(pw->pw_name),
> +		    (unsigned long)sizeof(li->username) - 1);
>  		return NULL;
>  	}

I don't think I have any warning which is too intrusive to fix.
Let me see, I've just started building the latest from CVS.

[...time passes...]

Here's the first one:

   ../../src/openbsd-compat/fmt_scaled.c:84:2: error: array subscript has type ‘char’ [-Werror=char-subscripts]
     while (isascii(*p) && isspace(*p))
     ^
 
  This is a warning message which has been deliberately introduced
  in newlib years ago.  Per POSIX, the arguments to the ctype functions
  or macros are

  "[...] an *int*, the value of which the application shall ensure is
   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*."

  See, for instance
  http://pubs.opengroup.org/onlinepubs/9699919799/functions/isalpha.html

  The problem is this:  If char is a signed type, the value of the
  character 0xff, when cast to int, is -1, which is in practically all
  cases == EOF.  That means, the character 0xff is mistreated as EOF,
  even if it's a valid char, if the application does not cast the input
  character to unsigned char.  And the character 0xff is a valid char in
  many codesets, as in most ISO-8859 variants.

  So, quite literally, this is wrong:

    char p;
    if (isalpha (p))

  and this is correct:

    char p;
    if (isalpha((unsigned char) p)

  The fix is to change all calls to the ctype functions so that an
  input of type char is casted to unsigned char, so that the conversion
  to int does not result in invalid values.

Patch is easy:

Index: openbsd-compat/fmt_scaled.c
===================================================================
RCS file: /cvs/openssh/openbsd-compat/fmt_scaled.c,v
retrieving revision 1.1
diff -u -p -r1.1 fmt_scaled.c
--- openbsd-compat/fmt_scaled.c	19 May 2008 22:57:08 -0000	1.1
+++ openbsd-compat/fmt_scaled.c	17 Jan 2014 14:21:57 -0000
@@ -55,7 +55,7 @@ typedef enum {
 
 /* These three arrays MUST be in sync!  XXX make a struct */
 static unit_type units[] = { NONE, KILO, MEGA, GIGA, TERA, PETA, EXA };
-static char scale_chars[] = "BKMGTPE";
+static unsigned char scale_chars[] = "BKMGTPE";
 static long long scale_factors[] = {
 	1LL,
 	1024LL,
@@ -75,7 +75,7 @@ static long long scale_factors[] = {
 int
 scan_scaled(char *scaled, long long *result)
 {
-	char *p = scaled;
+	unsigned char *p = scaled;
 	int sign = 0;
 	unsigned int i, ndigits = 0, fract_digits = 0;
 	long long scale_fact = 1, whole = 0, fpart = 0;
Index: openbsd-compat/readpassphrase.c
===================================================================
RCS file: /cvs/openssh/openbsd-compat/readpassphrase.c,v
retrieving revision 1.23
diff -u -p -r1.23 readpassphrase.c
--- openbsd-compat/readpassphrase.c	13 Jan 2010 10:32:44 -0000	1.23
+++ openbsd-compat/readpassphrase.c	17 Jan 2014 14:59:24 -0000
@@ -131,11 +131,11 @@ restart:
 			if (p < end) {
 				if ((flags & RPP_SEVENBIT))
 					ch &= 0x7f;
-				if (isalpha(ch)) {
+				if (isalpha((unsigned char )ch)) {
 					if ((flags & RPP_FORCELOWER))
-						ch = (char)tolower(ch);
+						ch = (char)tolower((unsigned char )ch);
 					if ((flags & RPP_FORCEUPPER))
-						ch = (char)toupper(ch);
+						ch = (char)toupper((unsigned char )ch);
 				}
 				*p++ = ch;
 			}

Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20140117/a8cf1e04/attachment-0001.bin>