Skip to content

Commit df6bde0

Browse files
committed
test: remove glibc fdelt sanity check
As is, this sanity check doesn't seem to be testing fdelt_chk, because passing a value of "0" to FD_SET wont cause the compiler to insert any calls to fdelt_chk(). The documentation is a little misleading. If we actually triggered fdelt_chk at runtime, bitcoind would abort. I think this check would be better replaced (if possible) by additional checks in security-check.py. The compiler may insert a call to fdelt_warn() (aliased with fdelt_chk in glibc) at compile time if it can determine that an invalid value is being passed to FD_SET. These checks are essentially; value < 0 or value >= FD_SETSIZE along with a check for wether the value is a compile time constant. If the compiler can determine an invalid value is being passed, a call to fdelt_warn will be inserted. Passing 0 should never cause a call to be inserted. You can check this after compiling: ```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) ``` To test, you could modify this test to pass -1 to FD_SET, and check that a call to fdelt_warn() is inserted, and that running bitcoind fails. i.e: ```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 ```
1 parent 8bf1540 commit df6bde0

File tree

4 files changed

+0
-68
lines changed

4 files changed

+0
-68
lines changed

configure.ac

Lines changed: 0 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -797,39 +797,6 @@ fi
797797

798798
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])
799799

800-
dnl FD_ZERO may be dependent on a declaration of memcpy, e.g. in SmartOS
801-
dnl check that it fails to build without memcpy, then that it builds with
802-
AC_MSG_CHECKING(FD_ZERO memcpy dependence)
803-
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
804-
#include <cstddef>
805-
#if HAVE_SYS_SELECT_H
806-
#include <sys/select.h>
807-
#endif
808-
]],[[
809-
#if HAVE_SYS_SELECT_H
810-
fd_set fds;
811-
FD_ZERO(&fds);
812-
#endif
813-
]])],
814-
[ AC_MSG_RESULT(no) ],
815-
[
816-
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
817-
#include <cstring>
818-
#if HAVE_SYS_SELECT_H
819-
#include <sys/select.h>
820-
#endif
821-
]], [[
822-
#if HAVE_SYS_SELECT_H
823-
fd_set fds;
824-
FD_ZERO(&fds);
825-
#endif
826-
]])],
827-
[ 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]) ],
828-
[ AC_MSG_ERROR(failed with cstring include) ]
829-
)
830-
]
831-
)
832-
833800
AC_CHECK_DECLS([getifaddrs, freeifaddrs],,,
834801
[#include <sys/types.h>
835802
#include <ifaddrs.h>]

src/Makefile.am

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -496,7 +496,6 @@ libbitcoin_util_a_SOURCES = \
496496
support/lockedpool.cpp \
497497
chainparamsbase.cpp \
498498
clientversion.cpp \
499-
compat/glibc_sanity_fdelt.cpp \
500499
compat/glibc_sanity.cpp \
501500
compat/glibcxx_sanity.cpp \
502501
compat/strnlen.cpp \

src/compat/glibc_sanity.cpp

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,6 @@
88

99
#include <cstddef>
1010

11-
#if defined(HAVE_SYS_SELECT_H)
12-
bool sanity_test_fdelt();
13-
#endif
14-
1511
extern "C" void* memcpy(void* a, const void* b, size_t c);
1612
void* memcpy_int(void* a, const void* b, size_t c)
1713
{
@@ -45,9 +41,5 @@ bool sanity_test_memcpy()
4541

4642
bool glibc_sanity_test()
4743
{
48-
#if defined(HAVE_SYS_SELECT_H)
49-
if (!sanity_test_fdelt())
50-
return false;
51-
#endif
5244
return sanity_test_memcpy<1025>();
5345
}

src/compat/glibc_sanity_fdelt.cpp

Lines changed: 0 additions & 26 deletions
This file was deleted.

0 commit comments

Comments
 (0)