foster/bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-03-05 14:06:27 -05:00
Code Issues Activity

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

Browse source 
df6bde031b test: remove glibc fdelt sanity check (fanquake)
8bf1540cc2 build: remove fdelt_chk backwards compatibility code (fanquake)

Pull request description:

  ae30d40e50
  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](https://github.com/bitcoin/bitcoin/pull/17538) we can remove our back-compat code.

  ab7bce584a
  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 87140d0c7..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  df6bde031b

Tree-SHA512: d8b3af4f4eb2d6c767ca6e72ece51d0ab9042e1bbdfcbbdb7ad713414df21489ba3217662b531b8bfdac0265d2ce5431abfae6e861b6187d182ff26c6e59b32d
This commit is contained in:
Wladimir J. van der Laan 2020-05-13 16:12:34 +02:00
commit 5d18c0ae18
No known key found for this signature in database
GPG key ID: 1E4AED62986CD25D
6 changed files with 2 additions and 100 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
build_msvc/bitcoin_config.h
View file

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

51
configure.ac
View file

@ -723,22 +723,8 @@ AX_GCC_FUNC_ATTRIBUTE([dllexport])
AX_GCC_FUNC_ATTRIBUTE([dllimport]) AX_GCC_FUNC_ATTRIBUTE([dllimport])
if test x$use_glibc_compat != xno; then if test x$use_glibc_compat != xno; then
AX_CHECK_LINK_FLAG([[-Wl,--wrap=__divmoddi4]], [COMPAT_LDFLAGS="$COMPAT_LDFLAGS -Wl,--wrap=__divmoddi4"])
dnl __fdelt_chk's params and return type have changed from long unsigned int to long int. AX_CHECK_LINK_FLAG([[-Wl,--wrap=log2f]], [COMPAT_LDFLAGS="$COMPAT_LDFLAGS -Wl,--wrap=log2f"])
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]])
else else
AC_SEARCH_LIBS([clock_gettime],[rt]) AC_SEARCH_LIBS([clock_gettime],[rt])
fi 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]) 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],,, AC_CHECK_DECLS([getifaddrs, freeifaddrs],,,
[#include <sys/types.h> [#include <sys/types.h>
#include <ifaddrs.h>] #include <ifaddrs.h>]

1
src/Makefile.am
View file

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

13
src/compat/glibc_compat.cpp
View file

@ -9,10 +9,6 @@
#include <cstddef> #include <cstddef>
#include <cstdint> #include <cstdint>
#if defined(HAVE_SYS_SELECT_H)
#include <sys/select.h>
#endif
// Prior to GLIBC_2.14, memcpy was aliased to memmove. // 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* memmove(void* a, const void* b, size_t c);
extern "C" void* memcpy(void* a, const void* b, size_t c) extern "C" void* memcpy(void* a, const void* b, size_t c)
@ -20,15 +16,6 @@ extern "C" void* memcpy(void* a, const void* b, size_t c)
return memmove(a, b, 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__) #if defined(__i386__) || defined(__arm__)
extern "C" int64_t __udivmoddi4(uint64_t u, uint64_t v, uint64_t* rp); extern "C" int64_t __udivmoddi4(uint64_t u, uint64_t v, uint64_t* rp);

8
src/compat/glibc_sanity.cpp
View file

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

26
src/compat/glibc_sanity_fdelt.cpp
View file

@ -1,26 +0,0 @@
// Copyright (c) 2009-2019 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endif
#if defined(HAVE_SYS_SELECT_H)
#ifdef HAVE_CSTRING_DEPENDENT_FD_ZERO
#include <cstring>
#endif
#include <sys/select.h>
// trigger: Call FD_SET to trigger __fdelt_chk. FORTIFY_SOURCE must be defined
// as >0 and optimizations must be set to at least -O2.
// test: Add a file descriptor to an empty fd_set. Verify that it has been
// correctly added.
bool sanity_test_fdelt()
{
fd_set fds;
FD_ZERO(&fds);
FD_SET(0, &fds);
return FD_ISSET(0, &fds);
}
#endif