build: fix sysctl() detection on macOS #18359

fanquake · 2020-03-16T09:43:24Z

sysctl() on *BSD takes a "const int *name", whereas sysctl() on macOS
it takes an "int *name". So our configure check and sysctl() detection on
macOS currently fails:

/usr/include/sys/sysctl.h:759:9: note: candidate function not viable:
	no known conversion from 'const int [2]' to 'int *' for 1st argument
int     sysctl(int *, u_int, void *, size_t *, void *, size_t);

The simplest change seems to be to change the param to a "int *name", which
will work during configure on macOS and *BSD systems.

For consistency I've changed both calls, but note that macOS doesn't
have KERN_ARND, so that check will always fail regardless. We can revert/add
documentation if preferred.

laanwj · 2020-03-16T10:44:45Z

ACK 155ce7f7b30a800b7b21f1965ee21967ffc18422
Changing both is good for consistency IMO.

practicalswift · 2020-03-16T11:22:21Z

Concept ACK

laanwj · 2020-03-17T16:07:36Z

Can someone on MacOS test this please and post results? (e.g. relevant configure output)

sipa · 2020-03-17T18:32:07Z

ACK after verifying that this is a no-op for MacOS.

dongcarl · 2020-03-17T19:11:10Z

Ran ./configure on OSX 10.14.6 before and after this change.

Before:

configure:24666: checking for sysctl
configure:24684: g++ -std=c++11 -c -g -O2  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -I/usr/local/opt/berkeley-db@4/include -DMAC_OSX -DOBJC_OLD_DISPATCH_PROTOTYPES=0 conftest.cpp >&5
conftest.cpp:73:5: error: no matching function for call to 'sysctl'
    sysctl(name, 2, nullptr, nullptr, nullptr, 0);
    ^~~~~~
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/sys/sysctl.h:759:9: note: candidate function not viable: no known conversion from 'const int [2]' to 'int *' for 1st argument
int     sysctl(int *, u_int, void *, size_t *, void *, size_t);
        ^
1 error generated.

After:

configure:24666: checking for sysctl
configure:24684: g++ -std=c++11 -c -g -O2  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -I/usr/local/opt/berkeley-db@4
/include -DMAC_OSX -DOBJC_OLD_DISPATCH_PROTOTYPES=0 conftest.cpp >&5
configure:24684: $? = 0
configure:24685: result: yes
configure:24696: checking for sysctl KERN_ARND
configure:24711: g++ -std=c++11 -c -g -O2  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -I/usr/local/opt/berkeley-db@4/include -DMAC_OSX -DOBJC_OLD_DISPATCH_PROTOTYPES=0 conftest.cpp >&5
conftest.cpp:70:34: error: use of undeclared identifier 'KERN_ARND'
 static int name[2] = {CTL_KERN, KERN_ARND};
                                 ^
1 error generated.

theuni · 2020-03-18T19:56:51Z

Concept ACK.

Shouldn't the actual usage in random.cpp be switched the more generic non-const version as well?

theuni · 2020-03-18T20:07:23Z

Actually... for the test, how about bypassing the issue:

sysctl(nullptr, 2, nullptr, nullptr, nullptr, 0);

?

sysctl() on *BSD takes a "const int *name", whereas sysctl() on macOS it takes an "int *name". So our configure check and sysctl() detection on macOS currently fails: ```bash /usr/include/sys/sysctl.h:759:9: note: candidate function not viable: no known conversion from 'const int [2]' to 'int *' for 1st argument int sysctl(int *, u_int, void *, size_t *, void *, size_t); ``` This change removes the name argument from the sysctl() detection check, meaning we will detect correctly on macOS and *BSD. For consistency we also switch to using the more generic, non-const version of the name parameter in the rest of our usage.

fanquake · 2020-03-19T04:02:37Z

@theuni Thanks for the comments / suggestions. I've pushed some changes.

Actually... for the test, how about bypassing the issue:

I've tested sysctl(nullptr, 2, nullptr, nullptr, nullptr, 0); for sysctl() detection in configure. It works on macOS and *BSDs. This seems like a good simplification.

Shouldn't the actual usage in random.cpp be switched the more generic non-const version as well?

There are two places we are using sysctl:

In randomenv.cpp we are already using the non-const version:

bitcoin/src/randomenv.cpp

Lines 166 to 171 in a421e0a

    
           void AddSysctl(CSHA512& hasher) 
        
           { 
        
               int CTL[sizeof...(S)] = {S...}; 
        
               unsigned char buffer[65536]; 
        
               size_t siz = 65536; 
        
               int ret = sysctl(CTL, sizeof...(S), buffer, &siz, nullptr, 0);

In random.cpp we are currently using the const int version:

bitcoin/src/random.cpp

Lines 323 to 331 in a421e0a

    
           #elif defined(HAVE_SYSCTL_ARND) 
        
               /* FreeBSD and similar. It is possible for the call to return less 
        
                * bytes than requested, so need to read in a loop. 
        
                */ 
        
               static const int name[2] = {CTL_KERN, KERN_ARND}; 
        
               int have = 0; 
        
               do { 
        
                   size_t len = NUM_OS_RANDOM_BYTES - have; 
        
                   if (sysctl(name, ARRAYLEN(name), ent32 + have, &len, nullptr, 0) != 0) {

Note that only FreeBSD and NetBSD should fall in here. We already special case macOS and OpenBSD for calls to getentropy(), and neither of them support KERN_ARND anyways. OpenBSD used to, but removed support in 6.1. I don't think macOS ever has.

Saying that, to be consistent in our sysctl() usage I've also changed this call to the non-const version.

I've tested e90e3e6 on the following platforms:

sysctl() detection:
FreeBSD 12.1
NetBSD 8.1
OpenBSD 6.6
macOS 10.14

KERN_ARND detection:
FreeBSD 12.1
NetBSD 8.1

DrahtBot · 2020-03-19T05:49:46Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

lib: fix a compiler warning: unused GetDevURandom() #17563 (lib: fix a compiler warning: unused GetDevURandom() by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj · 2020-03-19T14:19:23Z

That simplification is good though it does change the functionality of the test. Do we care whether CTL_KERN, KERN_VERSION and KERN_ARND are defined?

Edit: the check for KERN_VERSION is done separately in src/randomenv.cpp. So looks good to me.
Re-ACK e90e3e6

This has already been used to find one bug (bitcoin#18359), so turn it on by default.

e90e3e6 build: fix sysctl() detection on macOS (fanquake) Pull request description: [`sysctl()` on *BSD](https://www.unix.com/man-page/FreeBSD/3/sysctl/) takes a "const int *name", whereas [`sysctl()` on macOS](https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/sysctl.3.html) it takes an "int *name". So our configure check and `sysctl()` detection on macOS currently fails: ```bash /usr/include/sys/sysctl.h:759:9: note: candidate function not viable: no known conversion from 'const int [2]' to 'int *' for 1st argument int sysctl(int *, u_int, void *, size_t *, void *, size_t); ``` The simplest change seems to be to change the param to a "int *name", which will work during configure on macOS and *BSD systems. For consistency I've changed both calls, but note that macOS doesn't have `KERN_ARND`, so that check will always fail regardless. We can revert/add documentation if preferred. ACKs for top commit: laanwj: Re-ACK e90e3e6 Tree-SHA512: 29e9348136fc72882f63079bf10d2490e845d7656aae2c003e282bea49dd2778204a7776a67086bd88c2852af9a07dd04ba358eede7e37029e1c10f73c85d6a5

fanquake added the Build system label Mar 16, 2020

laanwj added this to the 0.20.0 milestone Mar 16, 2020

fanquake force-pushed the macos_sysctl branch from 155ce7f to e90e3e6 Compare March 19, 2020 04:01

DrahtBot mentioned this pull request Mar 19, 2020

lib: fix a compiler warning: unused GetDevURandom() #17563

Merged

laanwj merged commit 3d3d834 into bitcoin:master Mar 19, 2020

fanquake deleted the macos_sysctl branch March 20, 2020 02:01

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

fanquake added a commit to fanquake/bitcoin that referenced this pull request Mar 6, 2021

build: add -Wundef to default compile flags

8ebb656

This has already been used to find one bug (bitcoin#18359), so turn it on by default.

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

build: fix sysctl() detection on macOS #18359

build: fix sysctl() detection on macOS #18359

Uh oh!

fanquake commented Mar 16, 2020 •

edited

Loading

Uh oh!

laanwj commented Mar 16, 2020

Uh oh!

practicalswift commented Mar 16, 2020

Uh oh!

laanwj commented Mar 17, 2020 •

edited

Loading

Uh oh!

sipa commented Mar 17, 2020

Uh oh!

dongcarl commented Mar 17, 2020

Uh oh!

theuni commented Mar 18, 2020

Uh oh!

theuni commented Mar 18, 2020 •

edited

Loading

Uh oh!

fanquake commented Mar 19, 2020

Uh oh!

DrahtBot commented Mar 19, 2020

Uh oh!

laanwj commented Mar 19, 2020 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

build: fix sysctl() detection on macOS #18359

build: fix sysctl() detection on macOS #18359

Uh oh!

Conversation

fanquake commented Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Mar 16, 2020

Uh oh!

practicalswift commented Mar 16, 2020

Uh oh!

laanwj commented Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Mar 17, 2020

Uh oh!

dongcarl commented Mar 17, 2020

Uh oh!

theuni commented Mar 18, 2020

Uh oh!

theuni commented Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Mar 19, 2020

Uh oh!

DrahtBot commented Mar 19, 2020

Conflicts

Uh oh!

laanwj commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fanquake commented Mar 16, 2020 •

edited

Loading

laanwj commented Mar 17, 2020 •

edited

Loading

theuni commented Mar 18, 2020 •

edited

Loading

laanwj commented Mar 19, 2020 •

edited

Loading