Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix getentropy import check on osx #15103

Closed
wants to merge 1 commit into from

Conversation

@jameshilliard
Copy link
Contributor

@jameshilliard jameshilliard commented Jan 4, 2019

This should hopefully fix #15100.

@jameshilliard jameshilliard force-pushed the jameshilliard:getentropy-weak branch 10 times, most recently Jan 4, 2019
@jameshilliard jameshilliard changed the title declare getentropy weak import on osx fix getentropy import check on osx Jan 4, 2019
@jameshilliard jameshilliard force-pushed the jameshilliard:getentropy-weak branch Jan 4, 2019
@jameshilliard
Copy link
Contributor Author

@jameshilliard jameshilliard commented Jan 4, 2019

@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
Copy link
Contributor

@DesWurstes DesWurstes commented Jan 4, 2019

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
Copy link
Contributor Author

@jameshilliard jameshilliard commented Jan 4, 2019

@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
Copy link
Contributor

@DesWurstes DesWurstes commented Jan 4, 2019

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
Copy link
Contributor Author

@jameshilliard jameshilliard commented Jan 4, 2019

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
Copy link
Contributor

@DesWurstes DesWurstes commented Jan 5, 2019

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

  1. Please rename NULL to nullptr. It doesn't change anything, hence we should comply with style guide.

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

  3. 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));.

src/random.cpp Outdated Show resolved Hide resolved
@jameshilliard jameshilliard force-pushed the jameshilliard:getentropy-weak branch Jan 5, 2019
@jameshilliard
Copy link
Contributor Author

@jameshilliard jameshilliard commented Jan 5, 2019

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
Copy link
Contributor

@practicalswift practicalswift commented Jan 5, 2019

@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
Copy link
Contributor Author

@jameshilliard jameshilliard commented Jan 5, 2019

Could that be of any help? :-)

Doesn't seem to detect it correctly.

@jameshilliard
Copy link
Contributor Author

@jameshilliard jameshilliard commented Jan 5, 2019

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

@DesWurstes
Copy link
Contributor

@DesWurstes DesWurstes commented Jan 6, 2019

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
Copy link
Contributor

@practicalswift practicalswift commented Jan 6, 2019

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

@jameshilliard jameshilliard force-pushed the jameshilliard:getentropy-weak branch to 31d0853 Jan 9, 2019
@jameshilliard
Copy link
Contributor Author

@jameshilliard jameshilliard commented Jan 9, 2019

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 fanquake requested a review from theuni Jan 10, 2019
@laanwj
Copy link
Member

@laanwj laanwj commented Jan 14, 2019

utACK 31d0853

@jameshilliard jameshilliard force-pushed the jameshilliard:getentropy-weak branch from 31d0853 to a7c7fee Jan 19, 2019
@DesWurstes
Copy link
Contributor

@DesWurstes DesWurstes commented Jan 20, 2019

utACK a7c7fee

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

@jameshilliard
Copy link
Contributor Author

@jameshilliard jameshilliard commented Jan 20, 2019

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
Copy link
Member

@laanwj laanwj commented Jan 24, 2019

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
Copy link
Contributor Author

@jameshilliard jameshilliard commented Jan 24, 2019

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

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

@jameshilliard
Copy link
Contributor Author

@jameshilliard jameshilliard commented Feb 28, 2019

Anything holding this up?

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Mar 15, 2019

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
Copy link
Member

@fanquake fanquake commented Jul 29, 2019

@theuni Can you take a look here?

@theuni
Copy link
Member

@theuni theuni commented Aug 20, 2019

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
Copy link
Contributor Author

@jameshilliard jameshilliard commented Aug 21, 2019

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
Copy link
Contributor

@DesWurstes DesWurstes commented Aug 21, 2019

@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
	.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
Copy link
Member

@fanquake fanquake commented Dec 31, 2019

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 fanquake closed this Dec 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants