[PATCH v4 0/5] fix wrong program abort on __FD_ELT

[PATCH v4 0/5] fix wrong program abort on __FD_ELT

[KOSAKI Motohiro]

Changes from v3 to v4
 - remove _STRICT_FD_SIZE_CHECK ifdef.
 - instead, always check buffersize. requested from Florian Weimer.

Changes from v2 to v3
 - rebase to latest tree
 - remove [PATCH 1/6] mips: fix abi sort order
 - merge libc.abilist updates into [PATCH 2/4] Reinstantiate fd range check if and only if defined

Changes from v1 to v2
 - disable range check for pre-2.18 applications. It's required OpenSUSE and Ubuntu.


Currently, FD_SET, FD_CLR and FD_ISSET make program abort when
passing >__FD_SETSIZE value and _FORTIFY_SOURCE is greater than 0.

However it is wrong. Linux accept BSD style dynamic fd table allocations
likes below over 10 years. 

    http://netbsd.gw.com/cgi-bin/man-cgi?select++NetBSD-4.0

		   fd_set *fdsr;
                   int max = fd;

                   fdsr = (fd_set *)calloc(howmany(max+1, NFDBITS),
                       sizeof(fd_mask));
                   if (fdsr == NULL) {
                           ...
                           return (-1);
                   }
                   FD_SET(fd, fdsr);
                   n = select(max+1, fdsr, NULL, NULL, &tv);
                   ...
                   free(fdsr);

Moreover this technique is already in use multiple applications. And unfortunately, Ubuntu turn on _FORTIFY_SOURCE=2 by default and then user can hit this issue easily if bump up RLIMIT_NOFILE.

This patch series aim to exact buffer overflow check instead of hard coded 
FD_SETSIZE comparison. And after this series, an application which compiled with pre-2.18 no longer enable any boundary check for curing Ubuntu and OpenSUSE.

This series have following good characteristic.
 - Cure Ubuntu and OpenSUSE too.
 - Works both on stack and on heap fd_sets.
 - No additional compile time swich.

Please see a diff of tst-chk1.c (i.e. No change, Just add several tests). That said, new algorithm correctly detect all usage of the testcase. 


KOSAKI Motohiro (5):
  __fdelt_chk: Removed range check
  __FD_ELT: Implement correct buffer overflow check
  update libc.abilist
  tst-chk1: add fd_set dynamic allocation test
  __FDS_BITS: Added cast to __fd_mask* to avoid warning.

 ChangeLog                                          |   44 ++++++++++++++
 bits/select.h                                      |    6 +-
 debug/Versions                                     |    3 +
 debug/fdelt_chk.c                                  |   17 ++++-
 debug/tst-chk1.c                                   |   63 +++++++++++++++++++-
 misc/bits/select2.h                                |   23 ++++---
 misc/sys/select.h                                  |    6 +-
 ports/ChangeLog.aarch64                            |    5 ++
 ports/ChangeLog.alpha                              |    5 ++
 ports/ChangeLog.arm                                |    5 ++
 ports/ChangeLog.ia64                               |    5 ++
 ports/ChangeLog.m68k                               |    7 ++
 ports/ChangeLog.mips                               |    8 +++
 ports/ChangeLog.powerpc                            |    5 ++
 ports/ChangeLog.tile                               |    9 +++
 .../unix/sysv/linux/aarch64/nptl/libc.abilist      |    2 +
 .../unix/sysv/linux/alpha/nptl/libc.abilist        |    2 +
 .../sysdeps/unix/sysv/linux/arm/nptl/libc.abilist  |    2 +
 .../sysdeps/unix/sysv/linux/ia64/nptl/libc.abilist |    2 +
 .../sysv/linux/m68k/coldfire/nptl/libc.abilist     |    2 +
 .../unix/sysv/linux/m68k/m680x0/nptl/libc.abilist  |    2 +
 .../unix/sysv/linux/mips/mips32/nptl/libc.abilist  |    2 +
 .../sysv/linux/mips/mips64/n32/nptl/libc.abilist   |    2 +
 .../sysv/linux/mips/mips64/n64/nptl/libc.abilist   |    2 +
 .../powerpc/powerpc32/nofpu/nptl/libc.abilist      |    2 +
 .../linux/tile/tilegx/tilegx32/nptl/libc.abilist   |    2 +
 .../linux/tile/tilegx/tilegx64/nptl/libc.abilist   |    2 +
 .../unix/sysv/linux/tile/tilepro/nptl/libc.abilist |    2 +
 sysdeps/unix/sysv/linux/i386/nptl/libc.abilist     |    2 +
 .../linux/powerpc/powerpc32/fpu/nptl/libc.abilist  |    2 +
 .../sysv/linux/powerpc/powerpc64/nptl/libc.abilist |    2 +
 .../unix/sysv/linux/s390/s390-32/nptl/libc.abilist |    2 +
 .../unix/sysv/linux/s390/s390-64/nptl/libc.abilist |    2 +
 sysdeps/unix/sysv/linux/sh/nptl/libc.abilist       |    2 +
 .../sysv/linux/sparc/sparc32/nptl/libc.abilist     |    2 +
 .../sysv/linux/sparc/sparc64/nptl/libc.abilist     |    2 +
 .../unix/sysv/linux/x86_64/64/nptl/libc.abilist    |    2 +
 .../unix/sysv/linux/x86_64/x32/nptl/libc.abilist   |    2 +
 sysdeps/x86/bits/select.h                          |    6 +-
 39 files changed, 239 insertions(+), 24 deletions(-)

[PATCH 2/5] __FD_ELT: Implement correct buffer overflow check

[KOSAKI Motohiro]

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
---
 ChangeLog                 |   14 ++++++++++++++
 bits/select.h             |    6 +++---
 debug/Versions            |    3 +++
 debug/fdelt_chk.c         |   11 +++++++++++
 misc/bits/select2.h       |   23 +++++++++++++----------
 misc/sys/select.h         |    2 +-
 sysdeps/x86/bits/select.h |    6 +++---
 7 files changed, 48 insertions(+), 17 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 5311919..36bac6a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2013-04-13  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
+
+	* misc/sys/select.h (__FD_ELT): Add fdset new argument.
+	* bits/select.h (__FD_SET, __FD_CLR, __FD_ISSET): Adapt
+	an above change.
+	* sysdeps/x86/bits/select.h (__FD_SET, __FD_CLR, __FD_ISSET):
+	Likewise.
+
+	* debug/fdelt_chk.c (__fdelt_buffer_chk): New.
+	* misc/bits/select2.h (__FD_ELT): Implement correct buffer
+	overflow check instead of hardcoded FD_SET_SIZE.
+
+	* debug/Versions: Added __fdelt_buffer_chk and __fdelt_buffer_warn.
+
 2013-03-25  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
 
 	* debug/fdelt_chk.c (__fdelt_chk): Removed range check
diff --git a/bits/select.h b/bits/select.h
index ca87676..a10e18a 100644
--- a/bits/select.h
+++ b/bits/select.h
@@ -30,8 +30,8 @@
       __FDS_BITS (__arr)[__i] = 0;					      \
   } while (0)
 #define __FD_SET(d, s) \
-  ((void) (__FDS_BITS (s)[__FD_ELT(d)] |= __FD_MASK(d)))
+  ((void) (__FDS_BITS (s)[__FD_ELT(d, s)] |= __FD_MASK(d)))
 #define __FD_CLR(d, s) \
-  ((void) (__FDS_BITS (s)[__FD_ELT(d)] &= ~__FD_MASK(d)))
+  ((void) (__FDS_BITS (s)[__FD_ELT(d, s)] &= ~__FD_MASK(d)))
 #define __FD_ISSET(d, s) \
-  ((__FDS_BITS (s)[__FD_ELT (d)] & __FD_MASK (d)) != 0)
+  ((__FDS_BITS (s)[__FD_ELT (d, s)] & __FD_MASK (d)) != 0)
diff --git a/debug/Versions b/debug/Versions
index c1722fa..1ef2994 100644
--- a/debug/Versions
+++ b/debug/Versions
@@ -55,6 +55,9 @@ libc {
   GLIBC_2.16 {
     __poll_chk; __ppoll_chk;
   }
+  GLIBC_2.18 {
+    __fdelt_buffer_chk; __fdelt_buffer_warn;
+  }
   GLIBC_PRIVATE {
     __fortify_fail;
   }
diff --git a/debug/fdelt_chk.c b/debug/fdelt_chk.c
index 6588be0..8e16dc5 100644
--- a/debug/fdelt_chk.c
+++ b/debug/fdelt_chk.c
@@ -15,6 +15,7 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
+#include <sys/types.h>
 #include <sys/select.h>
 
 
@@ -25,3 +26,13 @@ __fdelt_nochk (long int d)
 }
 strong_alias (__fdelt_nochk, __fdelt_chk)
 strong_alias (__fdelt_nochk, __fdelt_warn)
+
+long int
+__fdelt_buffer_chk (long int fd, size_t buflen)
+{
+  if (fd / 8 >= buflen)
+    __chk_fail ();
+
+  return fd / __NFDBITS;;
+}
+strong_alias (__fdelt_buffer_chk, __fdelt_buffer_warn)
diff --git a/misc/bits/select2.h b/misc/bits/select2.h
index 03558c9..a0ffbb7 100644
--- a/misc/bits/select2.h
+++ b/misc/bits/select2.h
@@ -21,15 +21,18 @@
 #endif
 
 /* Helper functions to issue warnings and errors when needed.  */
-extern long int __fdelt_chk (long int __d);
-extern long int __fdelt_warn (long int __d)
+extern long int __fdelt_buffer_chk (long int __d, size_t buflen);
+extern long int __fdelt_buffer_warn (long int __d, size_t buflen)
   __warnattr ("bit outside of fd_set selected");
+
 #undef __FD_ELT
-#define	__FD_ELT(d) \
-  __extension__								    \
-  ({ long int __d = (d);						    \
-     (__builtin_constant_p (__d)					    \
-      ? (0 <= __d && __d < __FD_SETSIZE					    \
-	 ? (__d / __NFDBITS)						    \
-	 : __fdelt_warn (__d))						    \
-      : __fdelt_chk (__d)); })
+#define	__FD_ELT(d, s)							\
+  __extension__								\
+  ({									\
+    long int __d = (d);							\
+    (__bos0 (s) != (size_t) -1)						\
+      ? (__builtin_constant_p (__d) && ((__d / 8) >= __bos0 (s)))	\
+	? __fdelt_buffer_warn(__d, __bos0 (s))				\
+	: __fdelt_buffer_chk(__d, __bos0 (s))				\
+      : __d / __NFDBITS;						\
+  })
diff --git a/misc/sys/select.h b/misc/sys/select.h
index 21351fe..341190c 100644
--- a/misc/sys/select.h
+++ b/misc/sys/select.h
@@ -57,7 +57,7 @@ typedef long int __fd_mask;
 #undef	__NFDBITS
 /* It's easier to assume 8-bit bytes than to get CHAR_BIT.  */
 #define __NFDBITS	(8 * (int) sizeof (__fd_mask))
-#define	__FD_ELT(d)	((d) / __NFDBITS)
+#define	__FD_ELT(d, s)	((d) / __NFDBITS)
 #define	__FD_MASK(d)	((__fd_mask) 1 << ((d) % __NFDBITS))
 
 /* fd_set for select and pselect.  */
diff --git a/sysdeps/x86/bits/select.h b/sysdeps/x86/bits/select.h
index 8b87188..b29f301 100644
--- a/sysdeps/x86/bits/select.h
+++ b/sysdeps/x86/bits/select.h
@@ -56,8 +56,8 @@
 #endif	/* GNU CC */
 
 #define __FD_SET(d, set) \
-  ((void) (__FDS_BITS (set)[__FD_ELT (d)] |= __FD_MASK (d)))
+  ((void) (__FDS_BITS (set)[__FD_ELT (d, set)] |= __FD_MASK (d)))
 #define __FD_CLR(d, set) \
-  ((void) (__FDS_BITS (set)[__FD_ELT (d)] &= ~__FD_MASK (d)))
+  ((void) (__FDS_BITS (set)[__FD_ELT (d, set)] &= ~__FD_MASK (d)))
 #define __FD_ISSET(d, set) \
-  ((__FDS_BITS (set)[__FD_ELT (d)] & __FD_MASK (d)) != 0)
+  ((__FDS_BITS (set)[__FD_ELT (d, set)] & __FD_MASK (d)) != 0)
-- 
1.7.1

[PATCH 3/5] update libc.abilist

[KOSAKI Motohiro]

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
---
 ChangeLog                                          |   15 +++++++++++++++
 ports/ChangeLog.aarch64                            |    5 +++++
 ports/ChangeLog.alpha                              |    5 +++++
 ports/ChangeLog.arm                                |    5 +++++
 ports/ChangeLog.ia64                               |    5 +++++
 ports/ChangeLog.m68k                               |    7 +++++++
 ports/ChangeLog.mips                               |    8 ++++++++
 ports/ChangeLog.powerpc                            |    5 +++++
 ports/ChangeLog.tile                               |    9 +++++++++
 .../unix/sysv/linux/aarch64/nptl/libc.abilist      |    2 ++
 .../unix/sysv/linux/alpha/nptl/libc.abilist        |    2 ++
 .../sysdeps/unix/sysv/linux/arm/nptl/libc.abilist  |    2 ++
 .../sysdeps/unix/sysv/linux/ia64/nptl/libc.abilist |    2 ++
 .../sysv/linux/m68k/coldfire/nptl/libc.abilist     |    2 ++
 .../unix/sysv/linux/m68k/m680x0/nptl/libc.abilist  |    2 ++
 .../unix/sysv/linux/mips/mips32/nptl/libc.abilist  |    2 ++
 .../sysv/linux/mips/mips64/n32/nptl/libc.abilist   |    2 ++
 .../sysv/linux/mips/mips64/n64/nptl/libc.abilist   |    2 ++
 .../powerpc/powerpc32/nofpu/nptl/libc.abilist      |    2 ++
 .../linux/tile/tilegx/tilegx32/nptl/libc.abilist   |    2 ++
 .../linux/tile/tilegx/tilegx64/nptl/libc.abilist   |    2 ++
 .../unix/sysv/linux/tile/tilepro/nptl/libc.abilist |    2 ++
 sysdeps/unix/sysv/linux/i386/nptl/libc.abilist     |    2 ++
 .../linux/powerpc/powerpc32/fpu/nptl/libc.abilist  |    2 ++
 .../sysv/linux/powerpc/powerpc64/nptl/libc.abilist |    2 ++
 .../unix/sysv/linux/s390/s390-32/nptl/libc.abilist |    2 ++
 .../unix/sysv/linux/s390/s390-64/nptl/libc.abilist |    2 ++
 sysdeps/unix/sysv/linux/sh/nptl/libc.abilist       |    2 ++
 .../sysv/linux/sparc/sparc32/nptl/libc.abilist     |    2 ++
 .../sysv/linux/sparc/sparc64/nptl/libc.abilist     |    2 ++
 .../unix/sysv/linux/x86_64/64/nptl/libc.abilist    |    2 ++
 .../unix/sysv/linux/x86_64/x32/nptl/libc.abilist   |    2 ++
 32 files changed, 110 insertions(+), 0 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 36bac6a..7cdece7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -14,6 +14,21 @@
 
 2013-03-25  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
 
+	* sysdeps/unix/sysv/linux/i386/nptl/libc.abilist:
+	Add __fdelt_buffer_chk and __fdelt_buffer_warn.
+	* sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/nptl/libc.abilist:
+	Likewise.
+	* sysdeps/unix/sysv/linux/powerpc/powerpc64/nptl/libc.abilist: Likewise.
+	* sysdeps/unix/sysv/linux/s390/s390-32/nptl/libc.abilist: Likewise.
+	* sysdeps/unix/sysv/linux/s390/s390-64/nptl/libc.abilist: Likewise.
+	* sysdeps/unix/sysv/linux/sh/nptl/libc.abilist: Likewise.
+	* sysdeps/unix/sysv/linux/sparc/sparc32/nptl/libc.abilist: Likewise.
+	* sysdeps/unix/sysv/linux/sparc/sparc64/nptl/libc.abilist: Likewise.
+	* sysdeps/unix/sysv/linux/x86_64/64/nptl/libc.abilist: Likewise.
+	* sysdeps/unix/sysv/linux/x86_64/x32/nptl/libc.abilist: Likewise
+
+2013-03-25  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
+
 	* debug/fdelt_chk.c (__fdelt_chk): Removed range check
 	and renamed to __fdelt_nochk.
 
diff --git a/ports/ChangeLog.aarch64 b/ports/ChangeLog.aarch64
index f01388f..652f627 100644
--- a/ports/ChangeLog.aarch64
+++ b/ports/ChangeLog.aarch64
@@ -1,3 +1,8 @@
+2013-03-29  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
+
+	* ports/sysdeps/unix/sysv/linux/aarch64/nptl/libc.abilist:
+	Add __fdelt_check and __fdelt_buffer_warn.
+
 2013-03-19  Andreas Schwab  <schwab@suse.de>
 
 	* sysdeps/unix/sysv/linux/aarch64/configure.in: Set
diff --git a/ports/ChangeLog.alpha b/ports/ChangeLog.alpha
index 9a77d27..0afc097 100644
--- a/ports/ChangeLog.alpha
+++ b/ports/ChangeLog.alpha
@@ -1,3 +1,8 @@
+2013-03-29  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
+
+	* ports/sysdeps/unix/sysv/linux/alpha/nptl/libc.abilist:
+	Add __fdelt_check and __fdelt_buffer_warn.
+
 2013-03-06  Andreas Jaeger  <aj@suse.de>
 
 	* sysdeps/unix/sysv/linux/alpha/bits/mman.h (MAP_HUGE_MASK)
diff --git a/ports/ChangeLog.arm b/ports/ChangeLog.arm
index 355d980..d561038 100644
--- a/ports/ChangeLog.arm
+++ b/ports/ChangeLog.arm
@@ -1,3 +1,8 @@
+2013-03-29  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
+
+	* ports/sysdeps/unix/sysv/linux/arm/nptl/libc.abilist:
+	Add __fdelt_check and __fdelt_buffer_warn.
+
 2013-03-26  Mans Rullgard  <mans@mansr.com>
 
 	* sysdeps/arm/preconfigure.in: Use "test" instead of [ ].
diff --git a/ports/ChangeLog.ia64 b/ports/ChangeLog.ia64
index 50b5604..404d3fe 100644
--- a/ports/ChangeLog.ia64
+++ b/ports/ChangeLog.ia64
@@ -1,3 +1,8 @@
+2013-03-29  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
+
+	* ports/sysdeps/unix/sysv/linux/ia64/nptl/libc.abilist:
+	Add __fdelt_buffer_chk and __fdelt_buffer_warn.
+
 2013-03-12  Mike Frysinger  <vapier@gentoo.org>
 
 	* sysdeps/unix/sysv/linux/ia64/sysdep.h (INTERNAL_SYSCALL_DECL): Add
diff --git a/ports/ChangeLog.m68k b/ports/ChangeLog.m68k
index 16f3c75..e761662 100644
--- a/ports/ChangeLog.m68k
+++ b/ports/ChangeLog.m68k
@@ -1,3 +1,10 @@
+2013-03-29  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
+
+	* ports/sysdeps/unix/sysv/linux/m68k/coldfire/nptl/libc.abilist:
+	Add __fdelt_buffer_chk and __fdelt_buffer_warn.
+	* ports/sysdeps/unix/sysv/linux/m68k/m680x0/nptl/libc.abilist:
+	Likewise.
+
 2013-04-11  Andreas Schwab  <schwab@suse.de>
 
 	* sysdeps/m68k/m680x0/fpu/libm-test-ulps: Update
diff --git a/ports/ChangeLog.mips b/ports/ChangeLog.mips
index b221512..ec9899c 100644
--- a/ports/ChangeLog.mips
+++ b/ports/ChangeLog.mips
@@ -1,3 +1,11 @@
+2013-03-29  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
+	* ports/sysdeps/unix/sysv/linux/mips/mips32/nptl/libc.abilist:
+	Add __fdelt_buffer_chk and __fdelt_buffer_warn.
+	* ports/sysdeps/unix/sysv/linux/mips/mips64/n32/nptl/libc.abilist:
+	Likewise.
+	* ports/sysdeps/unix/sysv/linux/mips/mips64/n64/nptl/libc.abilist:
+	Likewise.
+
 2013-04-02  Thomas Schwinge  <thomas@codesourcery.com>
 
 	* sysdeps/mips/math_private.h: New file.
diff --git a/ports/ChangeLog.powerpc b/ports/ChangeLog.powerpc
index 2ba8e37..b9a11b1 100644
--- a/ports/ChangeLog.powerpc
+++ b/ports/ChangeLog.powerpc
@@ -1,3 +1,8 @@
+2013-03-29  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
+
+	* ports/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/nptl/libc.abilist:
+	Add __fdelt_buffer_chk and __fdelt_buffer_warn.
+
 2013-02-28  Joseph Myers  <joseph@codesourcery.com>
 
 	[BZ #13550]
diff --git a/ports/ChangeLog.tile b/ports/ChangeLog.tile
index f63bde4..ed13c14 100644
--- a/ports/ChangeLog.tile
+++ b/ports/ChangeLog.tile
@@ -1,3 +1,12 @@
+2013-03-29  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
+
+	* ports/sysdeps/unix/sysv/linux/tile/tilegx/tilegx32/nptl/libc.abilist:
+	Add __fdelt_buffer_chk and __fdelt_buffer_warn.
+	* ports/sysdeps/unix/sysv/linux/tile/tilegx/tilegx64/nptl/libc.abilist:
+	Likewise.
+	* ports/sysdeps/unix/sysv/linux/tile/tilepro/nptl/libc.abilist:
+	Likewise.
+
 2013-03-11  Andreas Schwab  <schwab@suse.de>
 
 	[BZ #15234]
diff --git a/ports/sysdeps/unix/sysv/linux/aarch64/nptl/libc.abilist b/ports/sysdeps/unix/sysv/linux/aarch64/nptl/libc.abilist
index b04a761..f21acac 100644
--- a/ports/sysdeps/unix/sysv/linux/aarch64/nptl/libc.abilist
+++ b/ports/sysdeps/unix/sysv/linux/aarch64/nptl/libc.abilist
@@ -2080,3 +2080,5 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
diff --git a/ports/sysdeps/unix/sysv/linux/alpha/nptl/libc.abilist b/ports/sysdeps/unix/sysv/linux/alpha/nptl/libc.abilist
index 980e088..f9d5437 100644
--- a/ports/sysdeps/unix/sysv/linux/alpha/nptl/libc.abilist
+++ b/ports/sysdeps/unix/sysv/linux/alpha/nptl/libc.abilist
@@ -1822,6 +1822,8 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
 GLIBC_2.2
  GLIBC_2.2 A
  _IO_adjust_wcolumn F
diff --git a/ports/sysdeps/unix/sysv/linux/arm/nptl/libc.abilist b/ports/sysdeps/unix/sysv/linux/arm/nptl/libc.abilist
index ce45208..ca82b49 100644
--- a/ports/sysdeps/unix/sysv/linux/arm/nptl/libc.abilist
+++ b/ports/sysdeps/unix/sysv/linux/arm/nptl/libc.abilist
@@ -89,6 +89,8 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
 GLIBC_2.4
  GLIBC_2.4 A
  _Exit F
diff --git a/ports/sysdeps/unix/sysv/linux/ia64/nptl/libc.abilist b/ports/sysdeps/unix/sysv/linux/ia64/nptl/libc.abilist
index 067552d..1d2dbb4 100644
--- a/ports/sysdeps/unix/sysv/linux/ia64/nptl/libc.abilist
+++ b/ports/sysdeps/unix/sysv/linux/ia64/nptl/libc.abilist
@@ -89,6 +89,8 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
 GLIBC_2.2
  GLIBC_2.2 A
  _Exit F
diff --git a/ports/sysdeps/unix/sysv/linux/m68k/coldfire/nptl/libc.abilist b/ports/sysdeps/unix/sysv/linux/m68k/coldfire/nptl/libc.abilist
index f06cc8e..8925116 100644
--- a/ports/sysdeps/unix/sysv/linux/m68k/coldfire/nptl/libc.abilist
+++ b/ports/sysdeps/unix/sysv/linux/m68k/coldfire/nptl/libc.abilist
@@ -90,6 +90,8 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
 GLIBC_2.4
  GLIBC_2.4 A
  _Exit F
diff --git a/ports/sysdeps/unix/sysv/linux/m68k/m680x0/nptl/libc.abilist b/ports/sysdeps/unix/sysv/linux/m68k/m680x0/nptl/libc.abilist
index 9010ea7..61e9820 100644
--- a/ports/sysdeps/unix/sysv/linux/m68k/m680x0/nptl/libc.abilist
+++ b/ports/sysdeps/unix/sysv/linux/m68k/m680x0/nptl/libc.abilist
@@ -1778,6 +1778,8 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
 GLIBC_2.2
  GLIBC_2.2 A
  _IO_adjust_wcolumn F
diff --git a/ports/sysdeps/unix/sysv/linux/mips/mips32/nptl/libc.abilist b/ports/sysdeps/unix/sysv/linux/mips/mips32/nptl/libc.abilist
index f01278e..863d3c9 100644
--- a/ports/sysdeps/unix/sysv/linux/mips/mips32/nptl/libc.abilist
+++ b/ports/sysdeps/unix/sysv/linux/mips/mips32/nptl/libc.abilist
@@ -1403,6 +1403,8 @@ GLIBC_2.18
  __cxa_thread_atexit_impl F
  __mips_fpu_getcw F
  __mips_fpu_setcw F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
 GLIBC_2.2
  GLIBC_2.2 A
  _Exit F
diff --git a/ports/sysdeps/unix/sysv/linux/mips/mips64/n32/nptl/libc.abilist b/ports/sysdeps/unix/sysv/linux/mips/mips64/n32/nptl/libc.abilist
index 9dbbd97..09d8b6c 100644
--- a/ports/sysdeps/unix/sysv/linux/mips/mips64/n32/nptl/libc.abilist
+++ b/ports/sysdeps/unix/sysv/linux/mips/mips64/n32/nptl/libc.abilist
@@ -1401,6 +1401,8 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
 GLIBC_2.2
  GLIBC_2.2 A
  _Exit F
diff --git a/ports/sysdeps/unix/sysv/linux/mips/mips64/n64/nptl/libc.abilist b/ports/sysdeps/unix/sysv/linux/mips/mips64/n64/nptl/libc.abilist
index c7e46aa..c87eda2 100644
--- a/ports/sysdeps/unix/sysv/linux/mips/mips64/n64/nptl/libc.abilist
+++ b/ports/sysdeps/unix/sysv/linux/mips/mips64/n64/nptl/libc.abilist
@@ -1399,6 +1399,8 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
 GLIBC_2.2
  GLIBC_2.2 A
  _Exit F
diff --git a/ports/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/nptl/libc.abilist b/ports/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/nptl/libc.abilist
index 9b6d663..2aabb21 100644
--- a/ports/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/nptl/libc.abilist
+++ b/ports/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/nptl/libc.abilist
@@ -1784,6 +1784,8 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
 GLIBC_2.2
  GLIBC_2.2 A
  _IO_adjust_wcolumn F
diff --git a/ports/sysdeps/unix/sysv/linux/tile/tilegx/tilegx32/nptl/libc.abilist b/ports/sysdeps/unix/sysv/linux/tile/tilegx/tilegx32/nptl/libc.abilist
index caf74b8..cb4ed3f 100644
--- a/ports/sysdeps/unix/sysv/linux/tile/tilegx/tilegx32/nptl/libc.abilist
+++ b/ports/sysdeps/unix/sysv/linux/tile/tilegx/tilegx32/nptl/libc.abilist
@@ -2091,3 +2091,5 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
diff --git a/ports/sysdeps/unix/sysv/linux/tile/tilegx/tilegx64/nptl/libc.abilist b/ports/sysdeps/unix/sysv/linux/tile/tilegx/tilegx64/nptl/libc.abilist
index 68d975b..1619bd7 100644
--- a/ports/sysdeps/unix/sysv/linux/tile/tilegx/tilegx64/nptl/libc.abilist
+++ b/ports/sysdeps/unix/sysv/linux/tile/tilegx/tilegx64/nptl/libc.abilist
@@ -2091,3 +2091,5 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
diff --git a/ports/sysdeps/unix/sysv/linux/tile/tilepro/nptl/libc.abilist b/ports/sysdeps/unix/sysv/linux/tile/tilepro/nptl/libc.abilist
index caf74b8..cb4ed3f 100644
--- a/ports/sysdeps/unix/sysv/linux/tile/tilepro/nptl/libc.abilist
+++ b/ports/sysdeps/unix/sysv/linux/tile/tilepro/nptl/libc.abilist
@@ -2091,3 +2091,5 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
diff --git a/sysdeps/unix/sysv/linux/i386/nptl/libc.abilist b/sysdeps/unix/sysv/linux/i386/nptl/libc.abilist
index 3cb314d..862741c 100644
--- a/sysdeps/unix/sysv/linux/i386/nptl/libc.abilist
+++ b/sysdeps/unix/sysv/linux/i386/nptl/libc.abilist
@@ -1822,6 +1822,8 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
 GLIBC_2.2
  GLIBC_2.2 A
  _IO_adjust_wcolumn F
diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/nptl/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/nptl/libc.abilist
index f27b48b..a01fb95 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/nptl/libc.abilist
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/nptl/libc.abilist
@@ -1784,6 +1784,8 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
 GLIBC_2.2
  GLIBC_2.2 A
  _IO_adjust_wcolumn F
diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/nptl/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc64/nptl/libc.abilist
index 195b587..2ba788a 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/nptl/libc.abilist
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/nptl/libc.abilist
@@ -90,6 +90,8 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
 GLIBC_2.3
  GLIBC_2.3 A
  _Exit F
diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/nptl/libc.abilist b/sysdeps/unix/sysv/linux/s390/s390-32/nptl/libc.abilist
index b6256d5..2bd9694 100644
--- a/sysdeps/unix/sysv/linux/s390/s390-32/nptl/libc.abilist
+++ b/sysdeps/unix/sysv/linux/s390/s390-32/nptl/libc.abilist
@@ -1774,6 +1774,8 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
 GLIBC_2.2
  GLIBC_2.2 A
  _IO_adjust_wcolumn F
diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/nptl/libc.abilist b/sysdeps/unix/sysv/linux/s390/s390-64/nptl/libc.abilist
index 265f66d..bd791f1 100644
--- a/sysdeps/unix/sysv/linux/s390/s390-64/nptl/libc.abilist
+++ b/sysdeps/unix/sysv/linux/s390/s390-64/nptl/libc.abilist
@@ -95,6 +95,8 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
 GLIBC_2.2
  GLIBC_2.2 A
  _Exit F
diff --git a/sysdeps/unix/sysv/linux/sh/nptl/libc.abilist b/sysdeps/unix/sysv/linux/sh/nptl/libc.abilist
index a653292..e287797 100644
--- a/sysdeps/unix/sysv/linux/sh/nptl/libc.abilist
+++ b/sysdeps/unix/sysv/linux/sh/nptl/libc.abilist
@@ -95,6 +95,8 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
 GLIBC_2.2
  GLIBC_2.2 A
  _Exit F
diff --git a/sysdeps/unix/sysv/linux/sparc/sparc32/nptl/libc.abilist b/sysdeps/unix/sysv/linux/sparc/sparc32/nptl/libc.abilist
index 9defbdf..039871a 100644
--- a/sysdeps/unix/sysv/linux/sparc/sparc32/nptl/libc.abilist
+++ b/sysdeps/unix/sysv/linux/sparc/sparc32/nptl/libc.abilist
@@ -1779,6 +1779,8 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
 GLIBC_2.2
  GLIBC_2.2 A
  _IO_adjust_wcolumn F
diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/nptl/libc.abilist b/sysdeps/unix/sysv/linux/sparc/sparc64/nptl/libc.abilist
index 35987fa..dd25510 100644
--- a/sysdeps/unix/sysv/linux/sparc/sparc64/nptl/libc.abilist
+++ b/sysdeps/unix/sysv/linux/sparc/sparc64/nptl/libc.abilist
@@ -100,6 +100,8 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
 GLIBC_2.2
  GLIBC_2.2 A
  _Exit F
diff --git a/sysdeps/unix/sysv/linux/x86_64/64/nptl/libc.abilist b/sysdeps/unix/sysv/linux/x86_64/64/nptl/libc.abilist
index 914b590..a890c13 100644
--- a/sysdeps/unix/sysv/linux/x86_64/64/nptl/libc.abilist
+++ b/sysdeps/unix/sysv/linux/x86_64/64/nptl/libc.abilist
@@ -91,6 +91,8 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
 GLIBC_2.2.5
  GLIBC_2.2.5 A
  _Exit F
diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/nptl/libc.abilist b/sysdeps/unix/sysv/linux/x86_64/x32/nptl/libc.abilist
index 0f64c8d..6fe9b66 100644
--- a/sysdeps/unix/sysv/linux/x86_64/x32/nptl/libc.abilist
+++ b/sysdeps/unix/sysv/linux/x86_64/x32/nptl/libc.abilist
@@ -2089,3 +2089,5 @@ GLIBC_2.17
 GLIBC_2.18
  GLIBC_2.18 A
  __cxa_thread_atexit_impl F
+ __fdelt_buffer_chk F
+ __fdelt_buffer_warn F
-- 
1.7.1

[PATCH 1/5] __fdelt_chk: Removed range check

[KOSAKI Motohiro]

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
---
 ChangeLog         |    5 +++++
 debug/fdelt_chk.c |    8 +++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 36efa0b..5311919 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2013-03-25  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
+
+	* debug/fdelt_chk.c (__fdelt_chk): Removed range check
+	and renamed to __fdelt_nochk.
+
 2013-04-11  Carlos O'Donell  <carlos@redhat.com>
 
 	* math/libm-test.inc (cos_test): Fix PI/2 test.
diff --git a/debug/fdelt_chk.c b/debug/fdelt_chk.c
index d149476..6588be0 100644
--- a/debug/fdelt_chk.c
+++ b/debug/fdelt_chk.c
@@ -19,11 +19,9 @@
 
 
 long int
-__fdelt_chk (long int d)
+__fdelt_nochk (long int d)
 {
-  if (d < 0 || d >= FD_SETSIZE)
-    __chk_fail ();
-
   return d / __NFDBITS;
 }
-strong_alias (__fdelt_chk, __fdelt_warn)
+strong_alias (__fdelt_nochk, __fdelt_chk)
+strong_alias (__fdelt_nochk, __fdelt_warn)
-- 
1.7.1

[PATCH 5/5] __FDS_BITS: Added cast to __fd_mask* to avoid warning.

[KOSAKI Motohiro]

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
---
 ChangeLog         |    5 +++++
 misc/sys/select.h |    4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index b154c3c..c027ab8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -29,6 +29,11 @@
 
 2013-03-25  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
 
+	* misc/sys/select.h (__FDS_BITS): Added a cast to
+	__fd_mask* to avoid warning.
+
+2013-03-25  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
+
 	* debug/tst-chk1.c (do_test): Added tests for fd_set allocation
 	from heap.
 
diff --git a/misc/sys/select.h b/misc/sys/select.h
index 341190c..99d330f 100644
--- a/misc/sys/select.h
+++ b/misc/sys/select.h
@@ -67,10 +67,10 @@ typedef struct
        from the global namespace.  */
 #ifdef __USE_XOPEN
     __fd_mask fds_bits[__FD_SETSIZE / __NFDBITS];
-# define __FDS_BITS(set) ((set)->fds_bits)
+# define __FDS_BITS(set) ((__fd_mask*)((set)->fds_bits))
 #else
     __fd_mask __fds_bits[__FD_SETSIZE / __NFDBITS];
-# define __FDS_BITS(set) ((set)->__fds_bits)
+# define __FDS_BITS(set) ((__fd_mask*)((set)->__fds_bits))
 #endif
   } fd_set;
 
-- 
1.7.1

[PATCH 4/5] tst-chk1: add fd_set dynamic allocation test

[KOSAKI Motohiro]

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
---
 ChangeLog        |    5 ++++
 debug/tst-chk1.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 67 insertions(+), 1 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 7cdece7..b154c3c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -29,6 +29,11 @@
 
 2013-03-25  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
 
+	* debug/tst-chk1.c (do_test): Added tests for fd_set allocation
+	from heap.
+
+2013-03-25  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
+
 	* debug/fdelt_chk.c (__fdelt_chk): Removed range check
 	and renamed to __fdelt_nochk.
 
diff --git a/debug/tst-chk1.c b/debug/tst-chk1.c
index 6ca8d9d..2b225ad 100644
--- a/debug/tst-chk1.c
+++ b/debug/tst-chk1.c
@@ -32,7 +32,7 @@
 #include <sys/select.h>
 #include <sys/socket.h>
 #include <sys/un.h>
-
+#include <sys/param.h>
 
 #define obstack_chunk_alloc malloc
 #define obstack_chunk_free free
@@ -1451,24 +1451,85 @@ do_test (void)
 #endif
 
   fd_set s;
+  fd_set *pfdset;
   FD_ZERO (&s);
   FD_SET (FD_SETSIZE - 1, &s);
 #if __USE_FORTIFY_LEVEL >= 1
   CHK_FAIL_START
   FD_SET (FD_SETSIZE, &s);
   CHK_FAIL_END
+
+  pfdset = (fd_set*)calloc(howmany(FD_SETSIZE+1, NFDBITS), sizeof(fd_mask));
+  if (pfdset == NULL) {
+    printf("malloc failed %m\n");
+    return 1;
+  }
+  FD_SET (FD_SETSIZE, pfdset);
+  free (pfdset);
+
+  pfdset = (fd_set*)calloc(howmany(FD_SETSIZE*2, NFDBITS), sizeof(fd_mask));
+  if (pfdset == NULL) {
+    printf("malloc failed %m\n");
+    return 1;
+  }
+  CHK_FAIL_START
+  FD_SET (FD_SETSIZE*2, pfdset);
+  CHK_FAIL_END
+  free (pfdset);
 #endif
   FD_CLR (FD_SETSIZE - 1, &s);
 #if __USE_FORTIFY_LEVEL >= 1
   CHK_FAIL_START
   FD_CLR (FD_SETSIZE, &s);
   CHK_FAIL_END
+
+  pfdset = (fd_set*)calloc(howmany(FD_SETSIZE+1, NFDBITS), sizeof(fd_mask));
+  if (pfdset == NULL) {
+    printf("malloc failed %m\n");
+    return 1;
+  }
+  FD_CLR (FD_SETSIZE, pfdset);
+
+  pfdset = (fd_set*)calloc(howmany(FD_SETSIZE*2, NFDBITS), sizeof(fd_mask));
+  if (pfdset == NULL) {
+    printf("malloc failed %m\n");
+    return 1;
+  }
+  free (pfdset);
+
+  pfdset = (fd_set*)calloc(howmany(FD_SETSIZE*2, NFDBITS), sizeof(fd_mask));
+  if (pfdset == NULL) {
+    printf("malloc failed %m\n");
+    return 1;
+  }
+  CHK_FAIL_START
+  FD_CLR (FD_SETSIZE*2, pfdset);
+  CHK_FAIL_END
+  free (pfdset);
 #endif
   FD_ISSET (FD_SETSIZE - 1, &s);
 #if __USE_FORTIFY_LEVEL >= 1
   CHK_FAIL_START
   FD_ISSET (FD_SETSIZE, &s);
   CHK_FAIL_END
+
+  pfdset = (fd_set*)calloc(howmany(FD_SETSIZE+1, NFDBITS), sizeof(fd_mask));
+  if (pfdset == NULL) {
+    printf("malloc failed %m\n");
+    return 1;
+  }
+  FD_ISSET (FD_SETSIZE, pfdset);
+  free (pfdset);
+
+  pfdset = (fd_set*)calloc(howmany(FD_SETSIZE*2, NFDBITS), sizeof(fd_mask));
+  if (pfdset == NULL) {
+    printf("malloc failed %m\n");
+    return 1;
+  }
+  CHK_FAIL_START
+  FD_ISSET (FD_SETSIZE*2, pfdset);
+  CHK_FAIL_END
+  free (pfdset);
 #endif
 
   struct pollfd fds[1];
-- 
1.7.1

Re: [PATCH 1/5] __fdelt_chk: Removed range check

[Carlos O'Donell]

On 04/13/2013 08:47 PM, KOSAKI Motohiro wrote:
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
> ---
>  ChangeLog         |    5 +++++
>  debug/fdelt_chk.c |    8 +++-----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 36efa0b..5311919 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,8 @@
> +2013-03-25  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
> +
> +	* debug/fdelt_chk.c (__fdelt_chk): Removed range check
> +	and renamed to __fdelt_nochk.
> +
>  2013-04-11  Carlos O'Donell  <carlos@redhat.com>
>  
>  	* math/libm-test.inc (cos_test): Fix PI/2 test.
> diff --git a/debug/fdelt_chk.c b/debug/fdelt_chk.c
> index d149476..6588be0 100644
> --- a/debug/fdelt_chk.c
> +++ b/debug/fdelt_chk.c
> @@ -19,11 +19,9 @@
>  
>  
>  long int
> -__fdelt_chk (long int d)
> +__fdelt_nochk (long int d)
>  {
> -  if (d < 0 || d >= FD_SETSIZE)
> -    __chk_fail ();
> -
>    return d / __NFDBITS;
>  }
> -strong_alias (__fdelt_chk, __fdelt_warn)
> +strong_alias (__fdelt_nochk, __fdelt_chk)
> +strong_alias (__fdelt_nochk, __fdelt_warn)
> 

Doesn't this mean that you will disable the runtime check
for FD_SETSIZE for all existing binaries?

That means that we would have to recompile all of the
applications again in order to get checking again using
the new symbols proposed in PATCH #2?

This is not sufficiently conservative. We want it the other
way around. A simple recompile of ruby should result in
a ruby that no longer needs to disable _FORTIFY_SOURCE
to work around FD_SETSIZE checks.

Cheers,
Carlos.

Re: [PATCH 2/5] __FD_ELT: Implement correct buffer overflow check

[Carlos O'Donell]

On 04/13/2013 08:47 PM, KOSAKI Motohiro wrote:
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
> ---
>  ChangeLog                 |   14 ++++++++++++++
>  bits/select.h             |    6 +++---
>  debug/Versions            |    3 +++
>  debug/fdelt_chk.c         |   11 +++++++++++
>  misc/bits/select2.h       |   23 +++++++++++++----------
>  misc/sys/select.h         |    2 +-
>  sysdeps/x86/bits/select.h |    6 +++---
>  7 files changed, 48 insertions(+), 17 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 5311919..36bac6a 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,17 @@
> +2013-04-13  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
> +
> +	* misc/sys/select.h (__FD_ELT): Add fdset new argument.
> +	* bits/select.h (__FD_SET, __FD_CLR, __FD_ISSET): Adapt
> +	an above change.
> +	* sysdeps/x86/bits/select.h (__FD_SET, __FD_CLR, __FD_ISSET):
> +	Likewise.
> +
> +	* debug/fdelt_chk.c (__fdelt_buffer_chk): New.
> +	* misc/bits/select2.h (__FD_ELT): Implement correct buffer
> +	overflow check instead of hardcoded FD_SET_SIZE.
> +
> +	* debug/Versions: Added __fdelt_buffer_chk and __fdelt_buffer_warn.
> +
>  2013-03-25  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
>  
>  	* debug/fdelt_chk.c (__fdelt_chk): Removed range check
> diff --git a/bits/select.h b/bits/select.h
> index ca87676..a10e18a 100644
> --- a/bits/select.h
> +++ b/bits/select.h
> @@ -30,8 +30,8 @@
>        __FDS_BITS (__arr)[__i] = 0;					      \
>    } while (0)
>  #define __FD_SET(d, s) \
> -  ((void) (__FDS_BITS (s)[__FD_ELT(d)] |= __FD_MASK(d)))
> +  ((void) (__FDS_BITS (s)[__FD_ELT(d, s)] |= __FD_MASK(d)))
>  #define __FD_CLR(d, s) \
> -  ((void) (__FDS_BITS (s)[__FD_ELT(d)] &= ~__FD_MASK(d)))
> +  ((void) (__FDS_BITS (s)[__FD_ELT(d, s)] &= ~__FD_MASK(d)))
>  #define __FD_ISSET(d, s) \
> -  ((__FDS_BITS (s)[__FD_ELT (d)] & __FD_MASK (d)) != 0)
> +  ((__FDS_BITS (s)[__FD_ELT (d, s)] & __FD_MASK (d)) != 0)
> diff --git a/debug/Versions b/debug/Versions
> index c1722fa..1ef2994 100644
> --- a/debug/Versions
> +++ b/debug/Versions
> @@ -55,6 +55,9 @@ libc {
>    GLIBC_2.16 {
>      __poll_chk; __ppoll_chk;
>    }
> +  GLIBC_2.18 {
> +    __fdelt_buffer_chk; __fdelt_buffer_warn;
> +  }
>    GLIBC_PRIVATE {
>      __fortify_fail;
>    }
> diff --git a/debug/fdelt_chk.c b/debug/fdelt_chk.c
> index 6588be0..8e16dc5 100644
> --- a/debug/fdelt_chk.c
> +++ b/debug/fdelt_chk.c
> @@ -15,6 +15,7 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> +#include <sys/types.h>
>  #include <sys/select.h>
>  
>  
> @@ -25,3 +26,13 @@ __fdelt_nochk (long int d)
>  }
>  strong_alias (__fdelt_nochk, __fdelt_chk)
>  strong_alias (__fdelt_nochk, __fdelt_warn)
> +
> +long int
> +__fdelt_buffer_chk (long int fd, size_t buflen)
> +{
> +  if (fd / 8 >= buflen)
> +    __chk_fail ();
> +
> +  return fd / __NFDBITS;;
> +}
> +strong_alias (__fdelt_buffer_chk, __fdelt_buffer_warn)
> diff --git a/misc/bits/select2.h b/misc/bits/select2.h
> index 03558c9..a0ffbb7 100644
> --- a/misc/bits/select2.h
> +++ b/misc/bits/select2.h
> @@ -21,15 +21,18 @@
>  #endif
>  
>  /* Helper functions to issue warnings and errors when needed.  */
> -extern long int __fdelt_chk (long int __d);
> -extern long int __fdelt_warn (long int __d)
> +extern long int __fdelt_buffer_chk (long int __d, size_t buflen);
> +extern long int __fdelt_buffer_warn (long int __d, size_t buflen)
>    __warnattr ("bit outside of fd_set selected");
> +
>  #undef __FD_ELT
> -#define	__FD_ELT(d) \
> -  __extension__								    \
> -  ({ long int __d = (d);						    \
> -     (__builtin_constant_p (__d)					    \
> -      ? (0 <= __d && __d < __FD_SETSIZE					    \
> -	 ? (__d / __NFDBITS)						    \
> -	 : __fdelt_warn (__d))						    \
> -      : __fdelt_chk (__d)); })
> +#define	__FD_ELT(d, s)							\
> +  __extension__								\
> +  ({									\
> +    long int __d = (d);							\
> +    (__bos0 (s) != (size_t) -1)						\
> +      ? (__builtin_constant_p (__d) && ((__d / 8) >= __bos0 (s)))	\
> +	? __fdelt_buffer_warn(__d, __bos0 (s))				\

Space between function and bracket e.g. foo () not foo().

> +	: __fdelt_buffer_chk(__d, __bos0 (s))				\
> +      : __d / __NFDBITS;						\

I'm not happy that this isn't very conservative.

If __bos0 fails should we fall back to static FD_SETSIZE checking
e.g. "__fdelt_buffer_warn (__d, FD_SETSIZE)"?

It seems that that would be better than no checking.

I know why you want to fall back to no check, because that
way you don't require any kind of new flag to disable the
check in the event it triggers when you don't want it to
(when __bos0 fails).

Does compiling ruby (or similar code) with this header
result in calls to __fdelt_buffer_warn or __fdelt_buffer_chk?

> +  })
> diff --git a/misc/sys/select.h b/misc/sys/select.h
> index 21351fe..341190c 100644
> --- a/misc/sys/select.h
> +++ b/misc/sys/select.h
> @@ -57,7 +57,7 @@ typedef long int __fd_mask;
>  #undef	__NFDBITS
>  /* It's easier to assume 8-bit bytes than to get CHAR_BIT.  */
>  #define __NFDBITS	(8 * (int) sizeof (__fd_mask))
> -#define	__FD_ELT(d)	((d) / __NFDBITS)
> +#define	__FD_ELT(d, s)	((d) / __NFDBITS)
>  #define	__FD_MASK(d)	((__fd_mask) 1 << ((d) % __NFDBITS))
>  
>  /* fd_set for select and pselect.  */
> diff --git a/sysdeps/x86/bits/select.h b/sysdeps/x86/bits/select.h
> index 8b87188..b29f301 100644
> --- a/sysdeps/x86/bits/select.h
> +++ b/sysdeps/x86/bits/select.h
> @@ -56,8 +56,8 @@
>  #endif	/* GNU CC */
>  
>  #define __FD_SET(d, set) \
> -  ((void) (__FDS_BITS (set)[__FD_ELT (d)] |= __FD_MASK (d)))
> +  ((void) (__FDS_BITS (set)[__FD_ELT (d, set)] |= __FD_MASK (d)))
>  #define __FD_CLR(d, set) \
> -  ((void) (__FDS_BITS (set)[__FD_ELT (d)] &= ~__FD_MASK (d)))
> +  ((void) (__FDS_BITS (set)[__FD_ELT (d, set)] &= ~__FD_MASK (d)))
>  #define __FD_ISSET(d, set) \
> -  ((__FDS_BITS (set)[__FD_ELT (d)] & __FD_MASK (d)) != 0)
> +  ((__FDS_BITS (set)[__FD_ELT (d, set)] & __FD_MASK (d)) != 0)
> 

Cheers,
Carlos.

Re: [PATCH 5/5] __FDS_BITS: Added cast to __fd_mask* to avoid warning.

[Carlos O'Donell]

On 04/13/2013 08:47 PM, KOSAKI Motohiro wrote:
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
> ---
>  ChangeLog         |    5 +++++
>  misc/sys/select.h |    4 ++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index b154c3c..c027ab8 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -29,6 +29,11 @@
>  
>  2013-03-25  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
>  
> +	* misc/sys/select.h (__FDS_BITS): Added a cast to
> +	__fd_mask* to avoid warning.
> +
> +2013-03-25  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
> +
>  	* debug/tst-chk1.c (do_test): Added tests for fd_set allocation
>  	from heap.
>  
> diff --git a/misc/sys/select.h b/misc/sys/select.h
> index 341190c..99d330f 100644
> --- a/misc/sys/select.h
> +++ b/misc/sys/select.h
> @@ -67,10 +67,10 @@ typedef struct
>         from the global namespace.  */
>  #ifdef __USE_XOPEN
>      __fd_mask fds_bits[__FD_SETSIZE / __NFDBITS];
> -# define __FDS_BITS(set) ((set)->fds_bits)
> +# define __FDS_BITS(set) ((__fd_mask*)((set)->fds_bits))
>  #else
>      __fd_mask __fds_bits[__FD_SETSIZE / __NFDBITS];
> -# define __FDS_BITS(set) ((set)->__fds_bits)
> +# define __FDS_BITS(set) ((__fd_mask*)((set)->__fds_bits))
>  #endif
>    } fd_set;
>  
> 

Obviously OK once we get the rest of the patches sorted.

Cheers,
Carlos.

Re: [PATCH 4/5] tst-chk1: add fd_set dynamic allocation test

[Carlos O'Donell]

On 04/13/2013 08:47 PM, KOSAKI Motohiro wrote:
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
> ---
>  ChangeLog        |    5 ++++
>  debug/tst-chk1.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 67 insertions(+), 1 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 7cdece7..b154c3c 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -29,6 +29,11 @@
>  
>  2013-03-25  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
>  
> +	* debug/tst-chk1.c (do_test): Added tests for fd_set allocation
> +	from heap.
> +
> +2013-03-25  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
> +
>  	* debug/fdelt_chk.c (__fdelt_chk): Removed range check
>  	and renamed to __fdelt_nochk.
>  
> diff --git a/debug/tst-chk1.c b/debug/tst-chk1.c
> index 6ca8d9d..2b225ad 100644
> --- a/debug/tst-chk1.c
> +++ b/debug/tst-chk1.c
> @@ -32,7 +32,7 @@
>  #include <sys/select.h>
>  #include <sys/socket.h>
>  #include <sys/un.h>
> -
> +#include <sys/param.h>
>  
>  #define obstack_chunk_alloc malloc
>  #define obstack_chunk_free free
> @@ -1451,24 +1451,85 @@ do_test (void)
>  #endif
>  
>    fd_set s;
> +  fd_set *pfdset;
>    FD_ZERO (&s);
>    FD_SET (FD_SETSIZE - 1, &s);
>  #if __USE_FORTIFY_LEVEL >= 1
>    CHK_FAIL_START
>    FD_SET (FD_SETSIZE, &s);
>    CHK_FAIL_END

Please add comments explaining *why* we are testing this usage
pattern e.g. Linux and BSDs support fds > FD_SETSIZE.

> +
> +  pfdset = (fd_set*)calloc(howmany(FD_SETSIZE+1, NFDBITS), sizeof(fd_mask));
> +  if (pfdset == NULL) {
> +    printf("malloc failed %m\n");
> +    return 1;
> +  }
> +  FD_SET (FD_SETSIZE, pfdset);
> +  free (pfdset);
> +
> +  pfdset = (fd_set*)calloc(howmany(FD_SETSIZE*2, NFDBITS), sizeof(fd_mask));
> +  if (pfdset == NULL) {
> +    printf("malloc failed %m\n");
> +    return 1;
> +  }
> +  CHK_FAIL_START
> +  FD_SET (FD_SETSIZE*2, pfdset);
> +  CHK_FAIL_END
> +  free (pfdset);
>  #endif
>    FD_CLR (FD_SETSIZE - 1, &s);
>  #if __USE_FORTIFY_LEVEL >= 1
>    CHK_FAIL_START
>    FD_CLR (FD_SETSIZE, &s);
>    CHK_FAIL_END
> +
> +  pfdset = (fd_set*)calloc(howmany(FD_SETSIZE+1, NFDBITS), sizeof(fd_mask));
> +  if (pfdset == NULL) {
> +    printf("malloc failed %m\n");
> +    return 1;
> +  }
> +  FD_CLR (FD_SETSIZE, pfdset);
> +
> +  pfdset = (fd_set*)calloc(howmany(FD_SETSIZE*2, NFDBITS), sizeof(fd_mask));
> +  if (pfdset == NULL) {
> +    printf("malloc failed %m\n");
> +    return 1;
> +  }
> +  free (pfdset);
> +
> +  pfdset = (fd_set*)calloc(howmany(FD_SETSIZE*2, NFDBITS), sizeof(fd_mask));
> +  if (pfdset == NULL) {
> +    printf("malloc failed %m\n");
> +    return 1;
> +  }
> +  CHK_FAIL_START
> +  FD_CLR (FD_SETSIZE*2, pfdset);
> +  CHK_FAIL_END
> +  free (pfdset);
>  #endif
>    FD_ISSET (FD_SETSIZE - 1, &s);
>  #if __USE_FORTIFY_LEVEL >= 1
>    CHK_FAIL_START
>    FD_ISSET (FD_SETSIZE, &s);
>    CHK_FAIL_END
> +
> +  pfdset = (fd_set*)calloc(howmany(FD_SETSIZE+1, NFDBITS), sizeof(fd_mask));
> +  if (pfdset == NULL) {
> +    printf("malloc failed %m\n");
> +    return 1;
> +  }
> +  FD_ISSET (FD_SETSIZE, pfdset);
> +  free (pfdset);
> +
> +  pfdset = (fd_set*)calloc(howmany(FD_SETSIZE*2, NFDBITS), sizeof(fd_mask));
> +  if (pfdset == NULL) {
> +    printf("malloc failed %m\n");
> +    return 1;
> +  }
> +  CHK_FAIL_START
> +  FD_ISSET (FD_SETSIZE*2, pfdset);
> +  CHK_FAIL_END
> +  free (pfdset);
>  #endif
>  
>    struct pollfd fds[1];
> 

Re: [PATCH v4 0/5] fix wrong program abort on __FD_ELT

[Carlos O'Donell]

On 04/13/2013 08:47 PM, KOSAKI Motohiro wrote:
> Changes from v3 to v4
>  - remove _STRICT_FD_SIZE_CHECK ifdef.
>  - instead, always check buffersize. requested from Florian Weimer.

Do we want to update manual/llio.texi to describe those macros
that can work with heap allocated fd sets?

These macros are being clearly used in Linux and BSD to operate
on heap allocated sets, and the glibc versions of some of these
macros do support those uses.

Cheers,
Carlos.

Re: [PATCH v4 0/5] fix wrong program abort on __FD_ELT

[KOSAKI Motohiro]

On Tue, Apr 30, 2013 at 11:08 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 04/13/2013 08:47 PM, KOSAKI Motohiro wrote:
>> Changes from v3 to v4
>>  - remove _STRICT_FD_SIZE_CHECK ifdef.
>>  - instead, always check buffersize. requested from Florian Weimer.
>
> Do we want to update manual/llio.texi to describe those macros
> that can work with heap allocated fd sets?
>
> These macros are being clearly used in Linux and BSD to operate
> on heap allocated sets, and the glibc versions of some of these
> macros do support those uses.

The manual already explains this case. I guess GNU/Hurd also support
the same extension.


@comment sys/types.h
@comment BSD
@deftypevr Macro int FD_SETSIZE
The value of this macro is the maximum number of file descriptors that a
@code{fd_set} object can hold information about.  On systems with a
fixed maximum number, @code{FD_SETSIZE} is at least that number.  On
some systems, including GNU, there is no absolute limit on the number of
descriptors open, but this macro still has a constant value which
controls the number of bits in an @code{fd_set}; if you get a file
descriptor with a value as high as @code{FD_SETSIZE}, you cannot put
that descriptor into an @code{fd_set}.
@end deftypevr

Re: [PATCH 2/5] __FD_ELT: Implement correct buffer overflow check

[KOSAKI Motohiro]

>> +     ? __fdelt_buffer_warn(__d, __bos0 (s))                          \
>
> Space between function and bracket e.g. foo () not foo().

ah, ok.

>
>> +     : __fdelt_buffer_chk(__d, __bos0 (s))                           \
>> +      : __d / __NFDBITS;                                             \
>
> I'm not happy that this isn't very conservative.
>
> If __bos0 fails should we fall back to static FD_SETSIZE checking
> e.g. "__fdelt_buffer_warn (__d, FD_SETSIZE)"?
>
> It seems that that would be better than no checking.

Hmm.. This doesn't cross my mind. All other buffer boundary checks
of _FORTIFY_SOURCE fall back no checking.  compiler may fails to
determine a right buffer size in various reasons. at that time, I don't
want to kill innocent applications.

> I know why you want to fall back to no check, because that
> way you don't require any kind of new flag to disable the
> check in the event it triggers when you don't want it to
> (when __bos0 fails).

If you like flag, I'm not putting objection. but if making flag, a lot
of libraries need
to turn on "no check" mode because when a buffer is allocated from applications,
library code can't know a buffer size at least at compile time.


> Does compiling ruby (or similar code) with this header
> result in calls to __fdelt_buffer_warn or __fdelt_buffer_chk?

Unfortunately, No. __builtin_object_size() require compiler know the
buffer size.
In the other words, it doesn't work if an allocate function and
FD_{SET,CLR} functions
doesn't exist in the same place. This is the same limitation with
other string buffer
overflow checks.

Re: [PATCH 4/5] tst-chk1: add fd_set dynamic allocation test

[KOSAKI Motohiro]

> Please add comments explaining *why* we are testing this usage
> pattern e.g. Linux and BSDs support fds > FD_SETSIZE.

OK.

Re: [PATCH 1/5] __fdelt_chk: Removed range check

[KOSAKI Motohiro]

>>  long int
>> -__fdelt_chk (long int d)
>> +__fdelt_nochk (long int d)
>>  {
>> -  if (d < 0 || d >= FD_SETSIZE)
>> -    __chk_fail ();
>> -
>>    return d / __NFDBITS;
>>  }
>> -strong_alias (__fdelt_chk, __fdelt_warn)
>> +strong_alias (__fdelt_nochk, __fdelt_chk)
>> +strong_alias (__fdelt_nochk, __fdelt_warn)
>>
>
> Doesn't this mean that you will disable the runtime check
> for FD_SETSIZE for all existing binaries?

Right.

> That means that we would have to recompile all of the
> applications again in order to get checking again using
> the new symbols proposed in PATCH #2?

Right. Because, unfortunately, __fdelt_chk() doesn't have
buffer size argument, so we can't implement buffer overflow
checks on top of this interface.

Then, I made new __fdelt_buffer_chk() function at patch #2.

The rest problem is, how should we treat old interfaces? From
point of Ubuntu and OpenSUSE view, it should be disable, at least,
by default. Otherwise all applications need to recompile for disabling.


> This is not sufficiently conservative. We want it the other
> way around. A simple recompile of ruby should result in
> a ruby that no longer needs to disable _FORTIFY_SOURCE
> to work around FD_SETSIZE checks.

If anyone have an alternative and better implementation idea, that's
welcome. I definitely agree this is ideal result.

Re: [PATCH v4 0/5] fix wrong program abort on __FD_ELT

[Carlos O'Donell]

On 05/01/2013 01:31 AM, KOSAKI Motohiro wrote:
> On Tue, Apr 30, 2013 at 11:08 PM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 04/13/2013 08:47 PM, KOSAKI Motohiro wrote:
>>> Changes from v3 to v4
>>>  - remove _STRICT_FD_SIZE_CHECK ifdef.
>>>  - instead, always check buffersize. requested from Florian Weimer.
>>
>> Do we want to update manual/llio.texi to describe those macros
>> that can work with heap allocated fd sets?
>>
>> These macros are being clearly used in Linux and BSD to operate
>> on heap allocated sets, and the glibc versions of some of these
>> macros do support those uses.
> 
> The manual already explains this case. I guess GNU/Hurd also support
> the same extension.
> 
> 
> @comment sys/types.h
> @comment BSD
> @deftypevr Macro int FD_SETSIZE
> The value of this macro is the maximum number of file descriptors that a
> @code{fd_set} object can hold information about.  On systems with a
> fixed maximum number, @code{FD_SETSIZE} is at least that number.  On
> some systems, including GNU, there is no absolute limit on the number of
> descriptors open, but this macro still has a constant value which
> controls the number of bits in an @code{fd_set}; if you get a file
> descriptor with a value as high as @code{FD_SETSIZE}, you cannot put
> that descriptor into an @code{fd_set}.
> @end deftypevr
> 

This should be expanded to say that at least on Linux you can allocate
space from the heap and describe which macros are safe to use in that
case (and what you need to do to avoid asserts from _FORTIFY_SOURCE).

Cheers,
Carlos.

Re: [PATCH 2/5] __FD_ELT: Implement correct buffer overflow check

[Carlos O'Donell]

On 05/01/2013 02:28 AM, KOSAKI Motohiro wrote:
>>> +     ? __fdelt_buffer_warn(__d, __bos0 (s))                          \
>>
>> Space between function and bracket e.g. foo () not foo().
> 
> ah, ok.
> 
>>
>>> +     : __fdelt_buffer_chk(__d, __bos0 (s))                           \
>>> +      : __d / __NFDBITS;                                             \
>>
>> I'm not happy that this isn't very conservative.
>>
>> If __bos0 fails should we fall back to static FD_SETSIZE checking
>> e.g. "__fdelt_buffer_warn (__d, FD_SETSIZE)"?
>>
>> It seems that that would be better than no checking.
> 
> Hmm.. This doesn't cross my mind. All other buffer boundary checks
> of _FORTIFY_SOURCE fall back no checking.  compiler may fails to
> determine a right buffer size in various reasons. at that time, I don't
> want to kill innocent applications.
> 
>> I know why you want to fall back to no check, because that
>> way you don't require any kind of new flag to disable the
>> check in the event it triggers when you don't want it to
>> (when __bos0 fails).
> 
> If you like flag, I'm not putting objection. but if making flag, a lot
> of libraries need
> to turn on "no check" mode because when a buffer is allocated from applications,
> library code can't know a buffer size at least at compile time.
> 
> 
>> Does compiling ruby (or similar code) with this header
>> result in calls to __fdelt_buffer_warn or __fdelt_buffer_chk?
> 
> Unfortunately, No. __builtin_object_size() require compiler know the
> buffer size.
> In the other words, it doesn't work if an allocate function and
> FD_{SET,CLR} functions
> doesn't exist in the same place. This is the same limitation with
> other string buffer
> overflow checks.

Then we need a flag, and ruby needs to use the flag to disable the
check on Linux.

The fundamental truth is that glibc implements POSIX, not "Linux."
And in POSIX there is a limit of FD_SETSIZE.

The default checking should be for POSIX.

We should provide a way to disable _FORTIFY_SOURCE checks that
are POSIX-only.

I still think your current macro is *better* because if __bos0
works then you have a dynamic check that is better than a static
check.

Thus the final solution is a combination of your new __bos0
changes and a flag to disable the check in the event that __bos0
fails.

What do you think?

Cheers,
Carlos.

Re: [PATCH 1/5] __fdelt_chk: Removed range check

[Carlos O'Donell]

On 05/01/2013 02:40 AM, KOSAKI Motohiro wrote:
>>>  long int
>>> -__fdelt_chk (long int d)
>>> +__fdelt_nochk (long int d)
>>>  {
>>> -  if (d < 0 || d >= FD_SETSIZE)
>>> -    __chk_fail ();
>>> -
>>>    return d / __NFDBITS;
>>>  }
>>> -strong_alias (__fdelt_chk, __fdelt_warn)
>>> +strong_alias (__fdelt_nochk, __fdelt_chk)
>>> +strong_alias (__fdelt_nochk, __fdelt_warn)
>>>
>>
>> Doesn't this mean that you will disable the runtime check
>> for FD_SETSIZE for all existing binaries?
> 
> Right.
> 
>> That means that we would have to recompile all of the
>> applications again in order to get checking again using
>> the new symbols proposed in PATCH #2?
> 
> Right. Because, unfortunately, __fdelt_chk() doesn't have
> buffer size argument, so we can't implement buffer overflow
> checks on top of this interface.
> 
> Then, I made new __fdelt_buffer_chk() function at patch #2.
> 
> The rest problem is, how should we treat old interfaces? From
> point of Ubuntu and OpenSUSE view, it should be disable, at least,
> by default. Otherwise all applications need to recompile for disabling.
> 
> 
>> This is not sufficiently conservative. We want it the other
>> way around. A simple recompile of ruby should result in
>> a ruby that no longer needs to disable _FORTIFY_SOURCE
>> to work around FD_SETSIZE checks.
> 
> If anyone have an alternative and better implementation idea, that's
> welcome. I definitely agree this is ideal result.
 
I don't think we want to disable the check.

We added it for good reasons and it matches POSIX behaviour.

At the end of the day we implement POSIX behaviour by default.

Ruby has already worked around this by disabling _FORTIFY_SOURCE
in their code to avoid the assert.

What we want to do is prevent them from needing to disable
*ALL* of _FORTIFY_SOURCE and provide a macro that allows them
to have finer grained control over the checks that apply to their
code.

Cheers,
Carlos.

Re: [PATCH 2/5] __FD_ELT: Implement correct buffer overflow check

[KOSAKI Motohiro]

>> Does compiling ruby (or similar code) with this header
>> result in calls to __fdelt_buffer_warn or __fdelt_buffer_chk?
> 
> Unfortunately, No. __builtin_object_size() require compiler know the
> buffer size.
> In the other words, it doesn't work if an allocate function and
> FD_{SET,CLR} functions
> doesn't exist in the same place. This is the same limitation with
> other string buffer
> overflow checks.

I inspected several other project codes.

sudo
------------------------
check_input(int ttyfd, double *speed)
{
(snip)
    fdsr = ecalloc(howmany(ttyfd + 1, NFDBITS), sizeof(fd_mask));
    for (;;) {
	FD_SET(ttyfd, fdsr);
	tv.tv_sec = 0;
	tv.tv_usec = 0;

	nready = select(ttyfd + 1, fdsr, NULL, NULL, paused ? NULL : &tv);
------------------------

In this case, __builtin_object_size() doesn't work too. However fixing is easy.
We only need to annotate ecalloc as attribute((alloc_size(1,2))).


rsyslog
-------------------------
BEGINobjConstruct(nsdsel_ptcp) /* be sure to specify the object type also in END macro! */
	pThis->maxfds = 0;
#ifdef USE_UNLIMITED_SELECT
        pThis->pReadfds = calloc(1, glbl.GetFdSetSize());
        pThis->pWritefds = calloc(1, glbl.GetFdSetSize());
#else
	FD_ZERO(&pThis->readfds);
	FD_ZERO(&pThis->writefds);
#endif
ENDobjConstruct(nsdsel_ptcp)
-------------------------

In this case, __builtin_object_size() doesn't work too. because pThis->p{Read,Write}fds can be
changed at runtime. compiler can't distingush the bitmap size.


openssh
------------------------
static int
timeout_connect(int sockfd, const struct sockaddr *serv_addr,
    socklen_t addrlen, int *timeoutp)
{
(snip)
	fdset = (fd_set *)xcalloc(howmany(sockfd + 1, NFDBITS),
	    sizeof(fd_mask));
	FD_SET(sockfd, fdset);
	ms_to_timeval(&tv, *timeoutp);

	for (;;) {
		rc = select(sockfd + 1, NULL, fdset, NULL, &tv);
------------------------


In this case, xcalloc also prevent to work __builtin_object_size(). It should be marded as attribute((alloc_size(1,2))).
but,

openssh 
------------------------
void
channel_prepare_select(fd_set **readsetp, fd_set **writesetp, int *maxfdp,
    u_int *nallocp, time_t *minwait_secs, int rekeying)
{
	u_int n, sz, nfdset;

	n = MAX(*maxfdp, channel_max_fd);

	nfdset = howmany(n+1, NFDBITS);
	/* Explicitly test here, because xrealloc isn't always called */
	if (nfdset && SIZE_T_MAX / nfdset < sizeof(fd_mask))
		fatal("channel_prepare_select: max_fd (%d) is too large", n);
	sz = nfdset * sizeof(fd_mask);

	/* perhaps check sz < nalloc/2 and shrink? */
	if (*readsetp == NULL || sz > *nallocp) {
		*readsetp = xrealloc(*readsetp, nfdset, sizeof(fd_mask));
		*writesetp = xrealloc(*writesetp, nfdset, sizeof(fd_mask));
		*nallocp = sz;
	}
	*maxfdp = n;
	memset(*readsetp, 0, sz);
	memset(*writesetp, 0, sz);

	if (!rekeying)
		channel_handler(channel_pre, *readsetp, *writesetp,
		    minwait_secs);
}
------------------------

This case is more harder. compiler can't know the size of readsetp and writesetp at
compilering caller functions.


librpcsecgss
------------------------
static int
readtcp(ct, buf, len)
	register struct ct_data *ct;
	caddr_t buf;
	register int len;
{
(snip)
	if (ct->ct_sock+1 > FD_SETSIZE) {
		int bytes = howmany(ct->ct_sock+1, NFDBITS) * sizeof(fd_mask);
		fds = (fd_set *)malloc(bytes);
		if (fds == NULL)
			return (-1);
		memset(fds, 0, bytes);
	} else {
		fds = &readfds;
		FD_ZERO(fds);
	}

	gettimeofday(&start, NULL);
	delta = ct->ct_wait;
	while (TRUE) {
		/* XXX we know the other bits are still clear */
		FD_SET(ct->ct_sock, fds);
		r = select(ct->ct_sock+1, fds, NULL, NULL, &delta);
		save_errno = errno;
--------------------------

This is ok. compiler can understand the size of fds.


bind9
-----------------------

static isc_result_t
setup_watcher(isc_mem_t *mctx, isc__socketmgr_t *manager) {
	isc_result_t result;
#if defined(USE_KQUEUE) || defined(USE_EPOLL) || defined(USE_DEVPOLL)
	char strbuf[ISC_STRERRORSIZE];
#endif
(snip)
#if ISC_SOCKET_MAXSOCKETS > FD_SETSIZE
	/*
	 * Note: this code should also cover the case of MAXSOCKETS <=
	 * FD_SETSIZE, but we separate the cases to avoid possible portability
	 * issues regarding howmany() and the actual representation of fd_set.
	 */
	manager->fd_bufsize = howmany(manager->maxsocks, NFDBITS) *
		sizeof(fd_mask);
#else
	manager->fd_bufsize = sizeof(fd_set);
#endif
---------------------------

This case also harder. FD_SET is called far different functions.



Summary: alomost software only need to add alloc_size() annotation to xmalloc() or 
similar in almost case. but there are several exceptions. some software have a complicated
fd size management and can't use __builtin_object_size(). but that's ok. In this case, the
software correctly expand buffers by realloc() or similar, so there is no chance to happen
buffer overflow.
 

Re: [PATCH 2/5] __FD_ELT: Implement correct buffer overflow check

[KOSAKI Motohiro]

>>> Does compiling ruby (or similar code) with this header
>>> result in calls to __fdelt_buffer_warn or __fdelt_buffer_chk?
>>
>> Unfortunately, No. __builtin_object_size() require compiler know the
>> buffer size.
>> In the other words, it doesn't work if an allocate function and
>> FD_{SET,CLR} functions
>> doesn't exist in the same place. This is the same limitation with
>> other string buffer
>> overflow checks.
> 
> Then we need a flag, and ruby needs to use the flag to disable the
> check on Linux.
>  
> The fundamental truth is that glibc implements POSIX, not "Linux."
> And in POSIX there is a limit of FD_SETSIZE.
> 
> The default checking should be for POSIX.
> 
> We should provide a way to disable _FORTIFY_SOURCE checks that
> are POSIX-only.
> 
> I still think your current macro is *better* because if __bos0
> works then you have a dynamic check that is better than a static
> check.
> 
> Thus the final solution is a combination of your new __bos0
> changes and a flag to disable the check in the event that __bos0
> fails.
> 
> What do you think?

Hmmm....

I'm puzzuled why you started to talk about ruby again. In ruby case,
recompilation and flag are ok. That's no problem.

But, as we've alread seen, several other software also uses the same technique.
and if not disable, Ubuntu need to recompile all of their packages. Do you
suggest to recompile all?

Moreover, IMHO fallbacking static check is completely useless because compiler
always can know the exact buffer size when using fd_set on stack. That's easy task
to distingush static array size form point of compiler view. In the other
hands, if compipler need to fall back, the buffer was allocated from heap in 99% 
case. and when using buffers allocated from heap, the size is larger than 1024
in almost all case. Then evetually, static check fallbacks makes false positive
aborting in almost all case.

Do you disagree?

I guess my conservative and your conservative are slightly different. My conservative
meant not to make false positive aborting. Your conservative seems preserve old behavior
as far as possible. In general, I agree with you. but in this case, I don't think __bos0()
fails to preserve to detect wrong FD_SET usage for buffers on stack. Do you have any 
specific (and practical) examples that my code fails to work?

# I know several hacky code _can_ trick my code. but I have not found practical and real world
# example.


But again, It's ok from ruby POV and I'm not argue if you really want to do it.

Re: [PATCH 1/5] __fdelt_chk: Removed range check

[KOSAKI Motohiro]

>>> That means that we would have to recompile all of the
>>> applications again in order to get checking again using
>>> the new symbols proposed in PATCH #2?
>>
>> Right. Because, unfortunately, __fdelt_chk() doesn't have
>> buffer size argument, so we can't implement buffer overflow
>> checks on top of this interface.
>>
>> Then, I made new __fdelt_buffer_chk() function at patch #2.
>>
>> The rest problem is, how should we treat old interfaces? From
>> point of Ubuntu and OpenSUSE view, it should be disable, at least,
>> by default. Otherwise all applications need to recompile for disabling.
>>
>>
>>> This is not sufficiently conservative. We want it the other
>>> way around. A simple recompile of ruby should result in
>>> a ruby that no longer needs to disable _FORTIFY_SOURCE
>>> to work around FD_SETSIZE checks.
>>
>> If anyone have an alternative and better implementation idea, that's
>> welcome. I definitely agree this is ideal result.
>  
> I don't think we want to disable the check.
> 
> We added it for good reasons and it matches POSIX behaviour.
> 
> At the end of the day we implement POSIX behaviour by default.

Could you clarify me my two questionsn? 

1) If not disabling, Ubuntu/OpenSUSE need to recompile ALL of affected 
packages. Do you suggest to recompile all of them? 

Dear Ubuntu and OpenSUSE guys, please tell me your opinion. This patch
doesn't affect Fedora/RHEL/Debian. So I want to know which is close to
your desire.

2) I think my code correctly catch buffer overflow of posix codes too.
Do you disagree it? In the other words, what kind of application wants 
to static checks?


> Ruby has already worked around this by disabling _FORTIFY_SOURCE
> in their code to avoid the assert.

Right. I don't mind ruby case.


> What we want to do is prevent them from needing to disable
> *ALL* of _FORTIFY_SOURCE and provide a macro that allows them
> to have finer grained control over the checks that apply to their
> code.

It can. but I want to understand which is better and why before to do.

Re: [PATCH v4 0/5] fix wrong program abort on __FD_ELT

[KOSAKI Motohiro]

>> @comment sys/types.h
>> @comment BSD
>> @deftypevr Macro int FD_SETSIZE
>> The value of this macro is the maximum number of file descriptors that a
>> @code{fd_set} object can hold information about.  On systems with a
>> fixed maximum number, @code{FD_SETSIZE} is at least that number.  On
>> some systems, including GNU, there is no absolute limit on the number of
>> descriptors open, but this macro still has a constant value which
>> controls the number of bits in an @code{fd_set}; if you get a file
>> descriptor with a value as high as @code{FD_SETSIZE}, you cannot put
>> that descriptor into an @code{fd_set}.
>> @end deftypevr
>>
> 
> This should be expanded to say that at least on Linux you can allocate
> space from the heap and describe which macros are safe to use in that
> case (and what you need to do to avoid asserts from _FORTIFY_SOURCE).

Hmm...
ok, I try to wording.

Re: [PATCH 1/5] __fdelt_chk: Removed range check

[Carlos O'Donell]

On 05/01/2013 06:13 PM, KOSAKI Motohiro wrote:
>>>> That means that we would have to recompile all of the
>>>> applications again in order to get checking again using
>>>> the new symbols proposed in PATCH #2?
>>>
>>> Right. Because, unfortunately, __fdelt_chk() doesn't have
>>> buffer size argument, so we can't implement buffer overflow
>>> checks on top of this interface.
>>>
>>> Then, I made new __fdelt_buffer_chk() function at patch #2.
>>>
>>> The rest problem is, how should we treat old interfaces? From
>>> point of Ubuntu and OpenSUSE view, it should be disable, at least,
>>> by default. Otherwise all applications need to recompile for disabling.
>>>
>>>
>>>> This is not sufficiently conservative. We want it the other
>>>> way around. A simple recompile of ruby should result in
>>>> a ruby that no longer needs to disable _FORTIFY_SOURCE
>>>> to work around FD_SETSIZE checks.
>>>
>>> If anyone have an alternative and better implementation idea, that's
>>> welcome. I definitely agree this is ideal result.
>>  
>> I don't think we want to disable the check.
>>
>> We added it for good reasons and it matches POSIX behaviour.
>>
>> At the end of the day we implement POSIX behaviour by default.
> 
> Could you clarify me my two questionsn? 

Adding distribution maintainers from:
http://sourceware.org/glibc/wiki/MAINTAINERS#Distribution_Maintainers
to the TO, including Andreas Jaegar (openSUSE) and 
Adam Conrad (Ubuntu(Canonical)).

> 1) If not disabling, Ubuntu/OpenSUSE need to recompile ALL of affected 
> packages. Do you suggest to recompile all of them? 

No. They need only recompile those applications that don't
conform to POSIX and expect a Linux-style select with support for
fds > 1024. Once recompiled we rely on __bos0 to make the
check dynamic, if it doesn't, then the package will need to disable
the check with a special fortify source flag.

At the end of the day the community has worked hard to enable
_FORTIFY_SOURCE. The distributions made a *conscious* choice to
turn on these checks. We are doing them a disservice if we disable
the checks in glibc because applications are having problems.

It was the distribution's choice to enable the check. The package
maintainers need to work with the distribution and glibc to understand
the issues and fix their code. Nothing is fixed by Ruby calling
glibc "wrong" e.g. "the implementation is wrong... it wrongly assume
fd is always less than FD_SETSIZE (i.e. 1024)...". It's not wrong,
it's POSIX ;-)
 
> Dear Ubuntu and OpenSUSE guys, please tell me your opinion. This patch
> doesn't affect Fedora/RHEL/Debian. So I want to know which is close to
> your desire.

Added Andreas and Adam to the TO.

> 2) I think my code correctly catch buffer overflow of posix codes too.
> Do you disagree it? In the other words, what kind of application wants 
> to static checks?

Your patch disables the check for existing binaries, therefore it
doesn't catch buffer overflow for existing binaries e.g. indexing
into the fds array with an out of bounds index.

The check enforces POSIX. It enforces an expectation for all
applications that expect POSIX behaviour, which is what glibc
implements.

The root of the problem is the question:
What does glibc implement? POSIX? or The Linux API?

The project implements POSIX and more, but where the two are in
conflict we need to work together to make an informed choice.

Here the consensus is coming around to:

* When compiling with _FORTIFY_SOURCE, accept a strict definition
  for the API and assume POSIX compliance.

* Allow an application to loosen the check if it expects Linux-style
  behaviour.

What kind of application wants static checks?

* Any application using `fd_set foo;' and conforming to POSIX.

There is a lot of example code that uses `fd_set xxx;' and then
manipulates xxx using the provided macros. That code benefits
from a static check. I'll bet, but have no proof, that you'd
find more of that code than code that malloc's the fd_set.
Which is the whole reason the default check in _FORTIFY_SOURCE
is to be conservative and check against <= FD_SETSIZE.

Does that answer your question?
 
>> Ruby has already worked around this by disabling _FORTIFY_SOURCE
>> in their code to avoid the assert.
> 
> Right. I don't mind ruby case.
> 
> 
>> What we want to do is prevent them from needing to disable
>> *ALL* of _FORTIFY_SOURCE and provide a macro that allows them
>> to have finer grained control over the checks that apply to their
>> code.
> 
> It can. but I want to understand which is better and why before to do.
> 
> 
> 

Re: [PATCH 2/5] __FD_ELT: Implement correct buffer overflow check

[Carlos O'Donell]

On 05/01/2013 04:31 PM, KOSAKI Motohiro wrote:
>>>> Does compiling ruby (or similar code) with this header
>>>> result in calls to __fdelt_buffer_warn or __fdelt_buffer_chk?
>>>
>>> Unfortunately, No. __builtin_object_size() require compiler know the
>>> buffer size.
>>> In the other words, it doesn't work if an allocate function and
>>> FD_{SET,CLR} functions
>>> doesn't exist in the same place. This is the same limitation with
>>> other string buffer
>>> overflow checks.
>>
>> Then we need a flag, and ruby needs to use the flag to disable the
>> check on Linux.
>>  
>> The fundamental truth is that glibc implements POSIX, not "Linux."
>> And in POSIX there is a limit of FD_SETSIZE.
>>
>> The default checking should be for POSIX.
>>
>> We should provide a way to disable _FORTIFY_SOURCE checks that
>> are POSIX-only.
>>
>> I still think your current macro is *better* because if __bos0
>> works then you have a dynamic check that is better than a static
>> check.
>>
>> Thus the final solution is a combination of your new __bos0
>> changes and a flag to disable the check in the event that __bos0
>> fails.
>>
>> What do you think?
> 
> Hmmm....
> 
> I'm puzzuled why you started to talk about ruby again. In ruby case,
> recompilation and flag are ok. That's no problem.

It's just an example.

> But, as we've alread seen, several other software also uses the same technique.
> and if not disable, Ubuntu need to recompile all of their packages. Do you
> suggest to recompile all?

Unfortunately yes, otherwise we devalue _FORTIFY_SOURCE.

> Moreover, IMHO fallbacking static check is completely useless because compiler
> always can know the exact buffer size when using fd_set on stack. That's easy task
> to distingush static array size form point of compiler view. In the other
> hands, if compipler need to fall back, the buffer was allocated from heap in 99% 
> case. and when using buffers allocated from heap, the size is larger than 1024
> in almost all case. Then evetually, static check fallbacks makes false positive
> aborting in almost all case.
> 
> Do you disagree?

I am worried that __bos0 will fail in a lot of cases, and yes, that will yield
a false positive, however it is *better* than what we had before, and that's
good.

I think this is a choice the distributions made, and _FORTIFY_SOURCE makes.

The application developers want Linux/BSD-style support, but _FORTIFY_SOURCE
by definition adheres to the stricter standard of POSIX.

I don't see a way to win other than:
* Attempt dynamic check.
* Attempt static check.
or
* Disable with flag.

You are suggesting:
* Attempt dynamic check.
* Skip check.

That devalues _FORTIFY_SOURCE. I would like to keep _FORTIFY_SOURCE as
strict as possible. Let the distributions make a choice about enabling
it, and give the packagers some options for loosening checks.

> I guess my conservative and your conservative are slightly different. My conservative
> meant not to make false positive aborting. Your conservative seems preserve old behavior
> as far as possible. In general, I agree with you. but in this case, I don't think __bos0()
> fails to preserve to detect wrong FD_SET usage for buffers on stack. Do you have any 
> specific (and practical) examples that my code fails to work?

There is some code somewhere that will cause __bos0() to fail.

In that situation *I* would rather a false positive than an overflow.

If you don't care about security don't compile with _FORTIFY_SOURCE?

> # I know several hacky code _can_ trick my code. but I have not found practical and real world
> # example.
> 
> 
> But again, It's ok from ruby POV and I'm not argue if you really want to do it.

I think your code is a better version of the existing code.

Cheers,
Carlos.

Re: [PATCH 2/5] __FD_ELT: Implement correct buffer overflow check

[Carlos O'Donell]

On 05/01/2013 04:11 PM, KOSAKI Motohiro wrote:
>>> Does compiling ruby (or similar code) with this header
>>> result in calls to __fdelt_buffer_warn or __fdelt_buffer_chk?
>>
>> Unfortunately, No. __builtin_object_size() require compiler know the
>> buffer size.
>> In the other words, it doesn't work if an allocate function and
>> FD_{SET,CLR} functions
>> doesn't exist in the same place. This is the same limitation with
>> other string buffer
>> overflow checks.
> 
> I inspected several other project codes.

Thank you very much for looking at these examples. They are quite
informative.

[snip]

> Summary: alomost software only need to add alloc_size() annotation to xmalloc() or 
> similar in almost case. but there are several exceptions. some software have a complicated
> fd size management and can't use __builtin_object_size(). but that's ok. In this case, the
> software correctly expand buffers by realloc() or similar, so there is no chance to happen
> buffer overflow.

So with your patch we enhance the number of cases that the check
is correct by using __bos0, and that's forward progress. I know
that it is less progress than you would like, but it is good
progress.

We keep _FORTIFY_SOURCE working usefully, even though it still
yields false positives.

The question as always with these checks is: Do you prefer false
positives or buffer overflows?

What's more harmful? The ecosystem thinking glibc and
the tools are "wrong" or buffer overflows leading to
security issues?

As a conservative project, and given the goal of _FORTIFY_SOURCE,
it seems like we have to leave the existing checks in place.

Cheers,
Carlos.