Merge #18862: Remove fdelt_chk back-compat code and sanity check

df6bde0 test: remove glibc fdelt sanity check (fanquake)
8bf1540 build: remove fdelt_chk backwards compatibility code (fanquake)

Pull request description:

  bitcoin/bitcoin@ae30d40
  The return type of [`fdelt_chk`](https://sourceware.org/git/?p=glibc.git;a=blob;f=debug/fdelt_chk.c;h=f62ce7349707cb68f55831c1c591fd7387a90258;hb=HEAD) changed from `unsigned  long int` to `long int` in glibc 2.16. See [this commit](https://sourceware.org/git/?p=glibc.git;a=commit;h=ceb9e56b3d1f8c1922e0526c2e841373843460e2). Now that we require [glibc >=2.17](bitcoin/bitcoin#17538) we can remove our back-compat code.

  bitcoin/bitcoin@ab7bce5
  While looking at the above changes, I noticed that our glibc fdelt sanity check doesn't seem to be checking anything. `fdelt_warn()` also isn't something we'd want to actually "trigger" at runtime, as doing so would cause `bitcoind` to abort.

  The comments:
  > // trigger: Call FD_SET to trigger __fdelt_chk. FORTIFY_SOURCE must be defined
  > //   as >0 and optimizations must be set to at least -O2.

  suggest calling FD_SET to check the invocation of `fdelt_chk` (this is [aliased with fdelt_warn in glibc](https://sourceware.org/git/?p=glibc.git;a=blob;f=debug/fdelt_chk.c;h=f62ce7349707cb68f55831c1c591fd7387a90258;hb=HEAD)). However just calling `FD_SET()` will not necessarily cause the compiler to insert a call to `fd_warn()`.

  Whether or not GCC (recent Clang should work, but may use different heuristics) inserts a call to `fdelt_warn()` depends on if the compiler can determine if the value passed in is a compile time constant (using [`__builtin_constant_p`](https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html)) and whether the value is < 0 or >= `FD_SETSIZE`. The glibc implementation is [here](https://sourceware.org/git/?p=glibc.git;a=blob;f=misc/bits/select2.h;h=7e17430ed94dd1679af10afa3d74795f9c97c0e8;hb=HEAD). This means our check should never cause a call to be inserted.

  Compiling master without `--glibc-back-compat` (if you do pass `--glibc-back-compat` the outcome is still the same; however the abort will only happen with >=`FD_SETSIZE` as that is what our [fdelt_warn()](https://github.com/bitcoin/bitcoin/blob/master/src/compat/glibc_compat.cpp#L24) checks for), there are no calls to `fdelt_warn()` inserted by the compiler:
  ```bash
  objdump -dC bitcoind | grep sanity_fdelt
  ...
  0000000000399d20 <sanity_test_fdelt()>:
    399d20:       48 81 ec 98 00 00 00    sub    $0x98,%rsp
    399d27:       b9 10 00 00 00          mov    $0x10,%ecx
    399d2c:       64 48 8b 04 25 28 00    mov    %fs:0x28,%rax
    399d33:       00 00
    399d35:       48 89 84 24 88 00 00    mov    %rax,0x88(%rsp)
    399d3c:       00
    399d3d:       31 c0                   xor    %eax,%eax
    399d3f:       48 89 e7                mov    %rsp,%rdi
    399d42:       fc                      cld
    399d43:       f3 48 ab                rep stos %rax,%es:(%rdi)
    399d46:       48 8b 84 24 88 00 00    mov    0x88(%rsp),%rax
    399d4d:       00
    399d4e:       64 48 33 04 25 28 00    xor    %fs:0x28,%rax
    399d55:       00 00
    399d57:       75 0d                   jne    399d66 <sanity_test_fdelt()+0x46>
    399d59:       b8 01 00 00 00          mov    $0x1,%eax
    399d5e:       48 81 c4 98 00 00 00    add    $0x98,%rsp
    399d65:       c3                      retq
    399d66:       e8 85 df c8 ff          callq  27cf0 <__stack_chk_fail@plt>
    399d6b:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)

  ```

  If you modify the sanity test to pass `-1` or `FD_SETSIZE` to `FD_SET`, you'll see calls to `fdelt_warn` inserted, and the runtime behaviour is an abort as expected.

  ```diff
  diff --git a/src/compat/glibc_sanity_fdelt.cpp b/src/compat/glibc_sanity_fdelt.cpp
  index 87140d0..16974bfa0 100644
  --- a/src/compat/glibc_sanity_fdelt.cpp
  +++ b/src/compat/glibc_sanity_fdelt.cpp
  @@ -20,7 +20,7 @@ bool sanity_test_fdelt()
   {
       fd_set fds;
       FD_ZERO(&fds);
  -    FD_SET(0, &fds);
  +    FD_SET(FD_SETSIZE, &fds);
       return FD_ISSET(0, &fds);
   }
   #endif
  ```

  ```bash
  0000000000399d20 <sanity_test_fdelt()>:
    399d20:	48 81 ec 98 00 00 00 	sub    $0x98,%rsp
    399d27:	b9 10 00 00 00       	mov    $0x10,%ecx
    399d2c:	64 48 8b 04 25 28 00 	mov    %fs:0x28,%rax
    399d33:	00 00
    399d35:	48 89 84 24 88 00 00 	mov    %rax,0x88(%rsp)
    399d3c:	00
    399d3d:	31 c0                	xor    %eax,%eax
    399d3f:	48 89 e7             	mov    %rsp,%rdi
    399d42:	fc                   	cld
    399d43:	f3 48 ab             	rep stos %rax,%es:(%rdi)
    399d46:	48 c7 c7 ff ff ff ff 	mov    $0xffffffffffffffff,%rdi
    399d4d:	e8 3e ff ff ff       	callq  399c90 <__fdelt_warn>
    399d52:	0f b6 04 24          	movzbl (%rsp),%eax
    399d56:	83 e0 01             	and    $0x1,%eax
    399d59:	48 8b 94 24 88 00 00 	mov    0x88(%rsp),%rdx
    399d60:	00
    399d61:	64 48 33 14 25 28 00 	xor    %fs:0x28,%rdx
    399d68:	00 00
    399d6a:	75 08                	jne    399d74 <sanity_test_fdelt()+0x54>
    399d6c:	48 81 c4 98 00 00 00 	add    $0x98,%rsp
    399d73:	c3                   	retq
    399d74:	e8 77 df c8 ff       	callq  27cf0 <__stack_chk_fail@plt>
    399d79:	0f 1f 80 00 00 00 00 	nopl   0x0(%rax)
   ```

   ```bash
   src/bitcoind
  *** buffer overflow detected ***: src/bitcoind terminated
  Aborted
   ```

  I think the test should should be removed and replaced (if possible) with additional checks in security-check.py. I was thinking about adding a version of [this script](https://github.com/fanquake/core-review/blob/master/fortify.py) as part of the output, but that needs more thought. I'll address this in a follow up.

ACKs for top commit:
  laanwj:
    ACK  df6bde0

Tree-SHA512: d8b3af4f4eb2d6c767ca6e72ece51d0ab9042e1bbdfcbbdb7ad713414df21489ba3217662b531b8bfdac0265d2ce5431abfae6e861b6187d182ff26c6e59b32d

Author: laanwj

build_msvc/bitcoin_config.h

@@ -41,9 +41,6 @@
 /* Define to 1 to enable ZMQ functions */
 #define ENABLE_ZMQ 1
 
-/* parameter and return value type for __fdelt_chk */
-/* #undef FDELT_TYPE */
-
 /* define if the Boost library is available */
 #define HAVE_BOOST /**/
 

configure.ac

@@ -723,22 +723,8 @@ AX_GCC_FUNC_ATTRIBUTE([dllexport])
 AX_GCC_FUNC_ATTRIBUTE([dllimport])
 
 if test x$use_glibc_compat != xno; then
-
-  dnl __fdelt_chk's params and return type have changed from long unsigned int to long int.
-  dnl See which one is present here.
-  AC_MSG_CHECKING(__fdelt_chk type)
-  AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#ifdef _FORTIFY_SOURCE
-                    #undef _FORTIFY_SOURCE
-                  #endif
-                  #define _FORTIFY_SOURCE 2
-                  #include <sys/select.h>
-     extern "C" long unsigned int __fdelt_warn(long unsigned int);]],[[]])],
-    [ fdelt_type="long unsigned int"],
-    [ fdelt_type="long int"])
-  AC_MSG_RESULT($fdelt_type)
-  AC_DEFINE_UNQUOTED(FDELT_TYPE, $fdelt_type,[parameter and return value type for __fdelt_chk])
-  AX_CHECK_LINK_FLAG([[-Wl,--wrap=__divmoddi4]], [COMPAT_LDFLAGS="$COMPAT_LDFLAGS -Wl,--wrap=__divmoddi4"],, [[$LDFLAG_WERROR]])
-  AX_CHECK_LINK_FLAG([[-Wl,--wrap=log2f]], [COMPAT_LDFLAGS="$COMPAT_LDFLAGS -Wl,--wrap=log2f"],, [[$LDFLAG_WERROR]])
+  AX_CHECK_LINK_FLAG([[-Wl,--wrap=__divmoddi4]], [COMPAT_LDFLAGS="$COMPAT_LDFLAGS -Wl,--wrap=__divmoddi4"])
+  AX_CHECK_LINK_FLAG([[-Wl,--wrap=log2f]], [COMPAT_LDFLAGS="$COMPAT_LDFLAGS -Wl,--wrap=log2f"])
 else
   AC_SEARCH_LIBS([clock_gettime],[rt])
 fi
@@ -817,39 +803,6 @@ fi
 
 AC_CHECK_HEADERS([endian.h sys/endian.h byteswap.h stdio.h stdlib.h unistd.h strings.h sys/types.h sys/stat.h sys/select.h sys/prctl.h sys/sysctl.h vm/vm_param.h sys/vmmeter.h sys/resources.h])
 
-dnl FD_ZERO may be dependent on a declaration of memcpy, e.g. in SmartOS
-dnl check that it fails to build without memcpy, then that it builds with
-AC_MSG_CHECKING(FD_ZERO memcpy dependence)
-AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
-    #include <cstddef>
-    #if HAVE_SYS_SELECT_H
-    #include <sys/select.h>
-    #endif
-  ]],[[
-    #if HAVE_SYS_SELECT_H
-    fd_set fds;
-    FD_ZERO(&fds);
-    #endif
-  ]])],
-  [ AC_MSG_RESULT(no) ],
-  [
-      AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
-          #include <cstring>
-          #if HAVE_SYS_SELECT_H
-          #include <sys/select.h>
-          #endif
-        ]], [[
-          #if HAVE_SYS_SELECT_H
-          fd_set fds;
-          FD_ZERO(&fds);
-          #endif
-        ]])],
-        [ AC_MSG_RESULT(yes); AC_DEFINE(HAVE_CSTRING_DEPENDENT_FD_ZERO, 1, [Define this symbol if FD_ZERO is dependent of a memcpy declaration being available]) ],
-        [ AC_MSG_ERROR(failed with cstring include) ]
-      )
-  ]
-)
-
 AC_CHECK_DECLS([getifaddrs, freeifaddrs],,,
     [#include <sys/types.h>
     #include <ifaddrs.h>]

src/Makefile.am

@@ -496,7 +496,6 @@ libbitcoin_util_a_SOURCES = \
   support/lockedpool.cpp \
   chainparamsbase.cpp \
   clientversion.cpp \
-  compat/glibc_sanity_fdelt.cpp \
   compat/glibc_sanity.cpp \
   compat/glibcxx_sanity.cpp \
   compat/strnlen.cpp \

src/compat/glibc_compat.cpp

@@ -9,26 +9,13 @@
 #include <cstddef>
 #include <cstdint>
 
-#if defined(HAVE_SYS_SELECT_H)
-#include <sys/select.h>
-#endif
-
 // Prior to GLIBC_2.14, memcpy was aliased to memmove.
 extern "C" void* memmove(void* a, const void* b, size_t c);
 extern "C" void* memcpy(void* a, const void* b, size_t c)
 {
     return memmove(a, b, c);
 }
 
-extern "C" void __chk_fail(void) __attribute__((__noreturn__));
-extern "C" FDELT_TYPE __fdelt_warn(FDELT_TYPE a)
-{
-    if (a >= FD_SETSIZE)
-        __chk_fail();
-    return a / __NFDBITS;
-}
-extern "C" FDELT_TYPE __fdelt_chk(FDELT_TYPE) __attribute__((weak, alias("__fdelt_warn")));
-
 #if defined(__i386__) || defined(__arm__)
 
 extern "C" int64_t __udivmoddi4(uint64_t u, uint64_t v, uint64_t* rp);

src/compat/glibc_sanity.cpp

@@ -8,10 +8,6 @@
 
 #include <cstddef>
 
-#if defined(HAVE_SYS_SELECT_H)
-bool sanity_test_fdelt();
-#endif
-
 extern "C" void* memcpy(void* a, const void* b, size_t c);
 void* memcpy_int(void* a, const void* b, size_t c)
 {
@@ -45,9 +41,5 @@ bool sanity_test_memcpy()
 
 bool glibc_sanity_test()
 {
-#if defined(HAVE_SYS_SELECT_H)
-    if (!sanity_test_fdelt())
-        return false;
-#endif
     return sanity_test_memcpy<1025>();
 }