fix getentropy import check on osx #15103

jameshilliard commented at 1:33 PM on January 4, 2019: contributor

This should hopefully fix #15100.

fanquake added the label macOS on Jan 4, 2019

fanquake added the label Utils/log/libs on Jan 4, 2019

jameshilliard force-pushed on Jan 4, 2019

jameshilliard renamed this:
~~declare getentropy weak import on osx~~
fix getentropy import check on osx
on Jan 4, 2019

jameshilliard force-pushed on Jan 4, 2019

jameshilliard commented at 3:48 PM on January 4, 2019: contributor

@DesWurstes Could you build and test this on OSX 10.10 to verify it works as expected, I only have a 10.14 system.

DesWurstes commented at 4:00 PM on January 4, 2019: contributor

On my slow computer, I only have Clang 3. I'll install brew, CLT, dependencies and build and test Bitcoin Core. Thus I need 24 hours to test it.

jameshilliard commented at 4:06 PM on January 4, 2019: contributor

@DesWurstes is it just your 10.10 system that's slow or your 10.14 as well? This should only need to be built on 10.14(but tested on 10.10 of course) since the build system disables getentropy for older SDK's.

DesWurstes commented at 4:08 PM on January 4, 2019: contributor

10.10 only. I've verified that this works on 10.10 when compiled with 10.14:

#include <stdio.h>
#include <sys/random.h>
extern int getentropy(void* buffer, size_t size) __attribute__((weak_import));

<<The same main function>>

Is compiling on 10.14 and running on 10.10 enough?

EDIT: Just the snippet.

EDIT: Will test tomorrow.

jameshilliard commented at 4:10 PM on January 4, 2019: contributor

Is compiling on 10.14 and running on 10.10 enough?

Should be since HAVE_GETENTROPY_RAND shouldn't get defined on 10.10. Did you verify bitcoin core itself works or just the test snippet?

DesWurstes commented at 2:18 PM on January 5, 2019: contributor

I can't test it. It needs too much work. Meanwhile:

Please rename NULL to nullptr. It doesn't change anything, hence we should comply with style guide.
It should check if attribute weak_import is supported by the compiler, and use it if so.
I looked at the header file and noticed that it has extern "C". Please change the prototype to extern "C" int getentropy(void *buffer, size_t size) __attribute__((weak_import));.

in src/random.cpp:248 in 8d5558970f outdated

 244 | @@ -244,7 +245,7 @@ void GetOSRand(unsigned char *ent32)
 245 |      }
 246 |  #elif defined(HAVE_GETENTROPY_RAND) && defined(MAC_OSX)
 247 |      // We need a fallback for OSX < 10.12
 248 | -    if (&getentropy != nullptr) {
 249 | +    if (&getentropy != NULL) {

practicalswift commented at 4:24 PM on January 5, 2019:

Please revert to nullptr :-)

jameshilliard force-pushed on Jan 5, 2019

jameshilliard commented at 8:13 PM on January 5, 2019: contributor

It should check if attribute weak_import is supported by the compiler, and use it if so.

Does that need to be a configure check?

practicalswift commented at 8:55 PM on January 5, 2019: contributor

~~@jameshilliard~~

#ifdef __has_cpp_attribute
#  if __has_cpp_attribute(weak_import)
…
#  endif
#endif

~~See https://github.com/bitcoin/bitcoin/blob/master/src/attributes.h as an example.~~

~~Could that be of any help? :-)~~

jameshilliard commented at 9:16 PM on January 5, 2019: contributor

Could that be of any help? :-)

Doesn't seem to detect it correctly.

jameshilliard commented at 9:23 PM on January 5, 2019: contributor

Do we actually need to add a check there? Unknown attributes only generate warnings right?

DesWurstes commented at 3:16 PM on January 6, 2019: contributor

If it is not supported, getentropy should be avoided and it should default to urandom.

__has cpp attributes is only for [[something]] attributes, with brackets.

practicalswift commented at 3:21 PM on January 6, 2019: contributor

@DesWurstes Oh, of course. Sorry for the brain fart. Thanks!

jameshilliard force-pushed on Jan 9, 2019

jameshilliard commented at 9:41 PM on January 9, 2019: contributor

I added a configure check for weak_import, would be good if someone could test against an OSX compiler without support for __attribute__((weak_import)), I'm not sure where to find one myself.

fanquake requested review from theuni on Jan 10, 2019

laanwj commented at 4:50 PM on January 14, 2019: member

utACK 31d0853c452e51707592f5736ddc6c76ef40f7fe

fix getentropy import check on osx a7c7fee2e4

jameshilliard force-pushed on Jan 19, 2019

DesWurstes commented at 6:11 AM on January 20, 2019: contributor

utACK https://github.com/bitcoin/bitcoin/commit/a7c7fee2e429178931160830a5c1233fbca3e024

~~Core members, please ACK or merge this before 0.18, or Core won't run on older Macs than 10.12.~~

jameshilliard commented at 6:50 AM on January 20, 2019: contributor

Core won't run on older Macs than 10.12.

Is xcode for gitian being bumped to at least version 8 for 0.18? The current xcode version being used AFAIK doesn't have getentropy support so gitian builds would not have the getentropy feature compiled in at all.

laanwj commented at 3:51 PM on January 24, 2019: member

Is xcode for gitian being bumped to at least version 8 for 0.18? The current xcode version being used AFAIK doesn't have getentropy support so gitian builds would not have the getentropy feature compiled in at all.

I don't think this needs to block this PR? I mean, getentropy should be handled correctly for user builds, or when the SDK version for gitian is bumped in the future.

jameshilliard commented at 9:20 PM on January 24, 2019: contributor

I don't think this needs to block this PR?

Yes, it's not something that would block this PR.

jameshilliard commented at 9:15 AM on February 28, 2019: contributor

Anything holding this up?

practicalswift commented at 4:38 PM on March 15, 2019: contributor

FWIW: I've verified that a disassembly of the bitcoind binary built on a Ubuntu 18.04 machine with this patch applied is identical to a disassembly of the bitcoind binary built against master (as expected).

fanquake commented at 1:40 AM on July 29, 2019: member

@theuni Can you take a look here?

theuni commented at 9:24 PM on August 20, 2019: member

This doesn't make any sense to me.

I don't see any need to try to override the symbol's export type, it's already marked weak as necessary in the SDK:

 $ grep getentropy SDKs/MacOSX10.14.sdk/usr/lib/system/libsystem_c.tbd 
                       '$ld$weak$os10.11$_dirname_r', '$ld$weak$os10.11$_getentropy',

I don't understand the history here. What was the actual reason for the revert in #15100? @MarcoFalke For your tests, did you remember to set -mmacosx-version-min in order to get the back compat at runtime?

jameshilliard commented at 12:13 AM on August 21, 2019: contributor

I don't see any need to try to override the symbol's export type, it's already marked weak as necessary in the SDK

Is that the case for the older SDK's as well?

DesWurstes commented at 7:21 AM on August 21, 2019: contributor

@theuni It's marked as weak in the ABI, not the API, so we also need to set it weak_import in the header. (If it weren't marked in the ABI, weak_import in the header wouldn't work, I believe). Since it's not in the API, the compiler doesn't add the checks and optimizes the if(!...) part. You can compile with -i to see the preprocessed code, which doesn't have weak_import.

On latest macOS, compiling

#include <stdio.h>
#include <sys/random.h>

int main(void) {
	unsigned char ent32[10];
	if (getentropy) puts("Have it!");
	else puts("Don't have it!");
	printf("%d\n", getentropy(ent32, 5));
	return 0;
}

with -Wall -Wextra -Wpedantic

GCC 9.2:

warning: the address of 'getentropy' will always evaluate as 'true' [-Waddress]
    6 |  if (getentropy) puts("Have it!");
      |      ^~~~~~~~~~

Clang 8.0.1:

warning: address of function
      'getentropy' will always evaluate to 'true' [-Wpointer-bool-conversion]
        if (getentropy) puts("Have it!");
        ~~  ^~~~~~~~~~

Apple LLVM 10.0.1:

warning: address of function
      'getentropy' will always evaluate to 'true' [-Wpointer-bool-conversion]
        if (getentropy) puts("Have it!");
        ~~  ^~~~~~~~~~

(Moreover, Clangs say:

Untitled.c:6:6: note: prefix with the address-of operator to silence this
      warning
        if (getentropy) puts("Have it!");

which is a trap we shouldn't fall into.)

Anyway, here's Clang's output with -S -fverbose-asm -O2: (Scroll down)

	.section	__TEXT,__text,regular,pure_instructions
	.macosx_version_min 10, 14
	.globl	_main                   ## -- Begin function main
	.p2align	4, 0x90
_main:                                  ## [@main](/bitcoin-bitcoin/contributor/main/)
	.cfi_startproc
## %bb.0:
	pushq	%rbp
	.cfi_def_cfa_offset 16
	.cfi_offset %rbp, -16
	movq	%rsp, %rbp
	.cfi_def_cfa_register %rbp
	subq	$32, %rsp
	movq	___stack_chk_guard@GOTPCREL(%rip), %rax
	movq	(%rax), %rax
	movq	%rax, -8(%rbp)
	leaq	L_.str(%rip), %rdi
	callq	_puts
	leaq	-18(%rbp), %rdi
	movl	$5, %esi
	callq	_getentropy
	leaq	L_.str.2(%rip), %rdi
	movl	%eax, %esi
	xorl	%eax, %eax
	callq	_printf
	movq	___stack_chk_guard@GOTPCREL(%rip), %rax
	movq	(%rax), %rax
	cmpq	-8(%rbp), %rax
	jne	LBB0_2
## %bb.1:
	xorl	%eax, %eax
	addq	$32, %rsp
	popq	%rbp
	retq
LBB0_2:
	callq	___stack_chk_fail
	.cfi_endproc
                                        ## -- End function
	.section	__TEXT,__cstring,cstring_literals
L_.str:                                 ## @.str
	.asciz	"Have it!"

L_.str.2:                               ## @.str.2
	.asciz	"%d\n"


.subsections_via_symbols

Notice the

L_.str:                                 ## @.str
	.asciz	"Have it!"

and "Don't have it!" was optimized away.

I had also verified that this crashed on my older mac after printing Have it!.

fanquake commented at 6:09 PM on December 31, 2019: member

I'm going to close this. It's not clear if this is the correct change, and, master now requires macOS >=10.12 (and will be compiled against the 10.14 SDK), which means that getentropy will always be available. If this is something that someone would still like to fix, please open a new PR against the 0.19 branch.

fanquake closed this on Dec 31, 2019

DrahtBot locked this on Dec 16, 2021