linux-kernel - Re: [PATCH v2] posix_types.h: make __NFDBITS compatible with glibc definition

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120724194301.GG10534@zod.bos.redhat.com>
Date:	Tue, 24 Jul 2012 15:43:01 -0400
From:	Josh Boyer <jwboyer@...hat.com>
To:	Jeff Law <law@...hat.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] posix_types.h: make __NFDBITS compatible with glibc
 definition

On Tue, Jul 24, 2012 at 01:26:39PM -0600, Jeff Law wrote:
> On 07/24/12 13:24, Linus Torvalds wrote:
> >On Tue, Jul 24, 2012 at 12:15 PM, Jeff Law <law@...hat.com> wrote:
> >>
> >>Please refer to the original discussion where they did evaluate the cost of
> >>this change and tested that the final change made no difference to the
> >>generated code.
> >
> >Umm. That bugzilla entry seems to be talking about a *sane* change, namely
> >
> >-  ({ unsigned long int __d = (d);					    \
> >+  ({ unsigned long int __d = (unsigned long int) (d);			    \
> >
> >in __FD_ELT(), which is totally different from the one Josh talks about.

So glibc has multiple definitions of __FD_ELT.  I originally quoted the
one from misc/sys/select.h, but the one from the first patch in the
glibc bugzilla entry is patching misc/bits/select2.h.  I'm going to
guess that through some kind of implies or header chain, the second is used.

However, the actually commit for that glibc bug is ceb9e56b3d1f8 and
that actually doesn't keep (unsigned long) here.  It does this:

diff --git a/debug/fdelt_chk.c b/debug/fdelt_chk.c
index 5e06f8f..ded3f2f 100644
--- a/debug/fdelt_chk.c
+++ b/debug/fdelt_chk.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 2011 Free Software Foundation, Inc.
+/* Copyright (C) 2011, 2012 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -18,10 +18,10 @@
 #include <sys/select.h>
 
 
-unsigned long int
-__fdelt_chk (unsigned long int d)
+long int
+__fdelt_chk (long int d)
 {
-  if (d >= FD_SETSIZE)
+  if (d < 0 || d >= FD_SETSIZE)
     __chk_fail ();
 
   return d / __NFDBITS;
diff --git a/misc/bits/select2.h b/misc/bits/select2.h
index 9679925..76ae368 100644
--- a/misc/bits/select2.h
+++ b/misc/bits/select2.h
@@ -1,5 +1,5 @@
 /* Checking macros for select functions.
-   Copyright (C) 2011 Free Software Foundation, Inc.
+   Copyright (C) 2011, 2012 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -21,14 +21,15 @@
 #endif
 
 /* Helper functions to issue warnings and errors when needed.  */
-extern unsigned long int __fdelt_chk (unsigned long int __d);
-extern unsigned long int __fdelt_warn (unsigned long int __d)
+extern long int __fdelt_chk (long int __d);
+extern long int __fdelt_warn (long int __d)
   __warnattr ("bit outside of fd_set selected");
 #undef __FD_ELT
 #define	__FD_ELT(d) \
   __extension__								    \
-  ({ unsigned long int __d = (d);					    \
+  ({ long int __d = (d);						    \
      (__builtin_constant_p (__d)					    \
-      ? (__d >= __FD_SETSIZE						    \
-	 ? __fdelt_warn (__d) : (__d / __NFDBITS))			    \
+      ? (0 <= __d && __d < __FD_SETSIZE					    \
+	 ? (__d / __NFDBITS)						    \
+	 : __fdelt_warn (__d))						    \
       : __fdelt_chk (__d)); })


> Right.  Josh's change is necessary to prevent warnings from folks
> (incorrectly) using posix_types.h instead of select.h after the
> change in that BZ was made.  That's why I originally stated that,
> arguably, posix_types.h really should go away or just use the
> definitions provided by glibc.

The warnings being specifically:

[root@...alhost ~]# cat foo.c 
#include <sys/select.h>
#include <linux/types.h>

int foo(void)
{
  fd_set fds;
  FD_ZERO(&fds);
  FD_SET(0, &fds);
  return FD_ISSET(0, &fds);
}

[root@...alhost ~]# gcc -Wextra -Werror -O2 -D_FORTIFY_SOURCE=2 -c foo.c -save-temps
foo.c: In function ‘foo’:
foo.c:8:162: error: signed and unsigned type in conditional expression [-Werror=sign-compare]
foo.c:8:184: error: signed and unsigned type in conditional expression [-Werror=sign-compare]
foo.c:9:162: error: signed and unsigned type in conditional expression [-Werror=sign-compare]
foo.c:9:184: error: signed and unsigned type in conditional expression [-Werror=sign-compare]
cc1: all warnings being treated as errors

As I said in the commit log, -Wsign-compare and -D_FORTIFY_SOURCE=2 are
required.  Now you have as much info as I do on this particular awesome
issue.

josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/