[go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

src/hostip.c: Move macOS-specific calls into global init call #11254

Closed
wants to merge 1 commit into from
Closed

src/hostip.c: Move macOS-specific calls into global init call #11254

wants to merge 1 commit into from

Conversation

stanhu
Copy link
Contributor
@stanhu stanhu commented Jun 5, 2023 •
edited
Loading

#7121 introduced a macOS system call to SCDynamicStoreCopyProxies, which is invoked every time an IP address needs to be resolved.

However, this system call is not thread-safe, and macOS will kill the process if the system call is run first in a fork. To make it possible for the parent process to call this once and prevent the crash, only invoke this system call in the global initialization routine.

In addition, this change is beneficial because it:

  1. Avoids extra macOS system calls for every IP lookup.
  2. Consolidates macOS-specific initialization in a separate file.

Closes #11252

@stanhu
Copy link
Contributor Author
stanhu commented Jun 5, 2023

@zajdee Could you take a look and see if this IPv6 -> IPv4 mapping still works here?

@stanhu stanhu force-pushed the sh-refactor-macos-init branch 2 times, most recently from d21e2b2 to 0f93c7e Compare June 5, 2023 17:50
@stanhu stanhu mentioned this pull request Jun 5, 2023
Closed
@zajdee
Copy link
zajdee commented Jun 5, 2023

It does look okay. Unfortunately I won't get to actually trying it sooner than tomorrow evening - would you be okay waiting until then?

@stanhu
Copy link
Contributor Author
stanhu commented Jun 5, 2023

@zajdee Sure, thanks.

@stanhu
Copy link
Contributor Author
stanhu commented Jun 8, 2023

@zajdee Would you mind confirming whether this change still works for the issue you were seeing with IPv6 mappings?

@curl curl deleted a comment Jun 8, 2023
bagder
bagder reviewed Jun 9, 2023
View reviewed changes
lib/macos.c Outdated Show resolved Hide resolved
lib/easy.c Outdated Show resolved Hide resolved
lib/macos.h Outdated Show resolved Hide resolved
@stanhu
src/hostip.c: Move macOS-specific calls into global init call
ddae69e 
curl#7121 introduced a macOS system call
to `SCDynamicStoreCopyProxies`, which is invoked every time an IP
address needs to be resolved.

However, this system call is not thread-safe, and macOS will kill the
process if the system call is run first in a fork. To make it possible
for the parent process to call this once and prevent the crash, only
invoke this system call in the global initialization routine.

In addition, this change is beneficial because it:

1. Avoids extra macOS system calls for every IP lookup.
2. Consolidates macOS-specific initialization in a separate file.

Closes curl#11252
@zajdee
Copy link
zajdee commented Jun 10, 2023

I have finally managed to find some time to test your branch. Doing the simplest:

autoreconf -fi
./configure --with-secure-transport
make

results in a missing symbol:

$ ./src/curl -h
dyld[97017]: symbol not found in flat namespace '_Curl_macos_init'
Abort trap: 6

I will be investigating further and let you know.
(The simplest autoreconf/configure/make builds a working curl when using the main curl branch from the curl/curl repository.)

@zajdee
Copy link
zajdee commented Jun 10, 2023 •
edited
Loading

Static build works as expected. I will investigate the dynamic build again later today (to rule out potential library conflicts on my side).

autoreconf -fi; ./configure --with-secure-transport --disable-shared; make

(Note the IPv6 address being used instead of the IPv4 one. This is a result of the NAT64/DNS64-enabled environment w/ NAT64 prefix fd00:64::/96 and CopyProxies being correctly called by the code.)

$ ./src/curl -v https://1.1.1.1/
*   Trying [fd00:64::101:101]:443...
* Connected to 1.1.1.1 (fd00:64::101:101) port 443 (#0)
* ALPN: offers h2,http/1.1
* WARNING: using IP address, SNI is being disabled by the OS.
* TLS 1.2 connection using TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
* Server certificate: cloudflare-dns.com
* Server certificate: DigiCert TLS Hybrid ECC SHA384 2020 CA1
* Server certificate: DigiCert Global Root CA
* using HTTP/2
* h2 [:method: GET]
* h2 [:scheme: https]
* h2 [:authority: 1.1.1.1]
* h2 [:path: /]
* h2 [user-agent: curl/8.2.0-DEV]
* h2 [accept: */*]
* Using Stream ID: 1 (easy handle 0x7f95dc80a800)
> GET / HTTP/2
> Host: 1.1.1.1
> User-Agent: curl/8.2.0-DEV
> Accept: */*

@stanhu
Copy link
Contributor Author
stanhu commented Jun 10, 2023

Thanks for confirming.

I repeated your steps and didn't run into the missing symbol issue.

@stanhu
Copy link
Contributor Author
stanhu commented Jun 27, 2023

@bagder Is there anything I can do to get this merged? Thanks so much for all your work on curl.

@zajdee
Copy link
zajdee commented Jun 27, 2023

Hi @stanhu,
just to confirm, after cleaning up my environment even dynamic builds work as expected. Thank you.

@bagder I support the merge of this change.

@bagder
Copy link
Member
bagder commented Jul 9, 2023

Thanks!

@bagder bagder closed this in c730859 Jul 9, 2023
bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
@stanhu @bch
hostip.c: Move macOS-specific calls into global init call
80739cb 
curl#7121 introduced a macOS system call
to `SCDynamicStoreCopyProxies`, which is invoked every time an IP
address needs to be resolved.

However, this system call is not thread-safe, and macOS will kill the
process if the system call is run first in a fork. To make it possible
for the parent process to call this once and prevent the crash, only
invoke this system call in the global initialization routine.

In addition, this change is beneficial because it:

1. Avoids extra macOS system calls for every IP lookup.
2. Consolidates macOS-specific initialization in a separate file.

Fixes curl#11252
Closes curl#11254
@fds242 fds242 mentioned this pull request Jul 25, 2023
Closed
ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
@stanhu @ptitSeb
hostip.c: Move macOS-specific calls into global init call
21c84fe 
curl#7121 introduced a macOS system call
to `SCDynamicStoreCopyProxies`, which is invoked every time an IP
address needs to be resolved.

However, this system call is not thread-safe, and macOS will kill the
process if the system call is run first in a fork. To make it possible
for the parent process to call this once and prevent the crash, only
invoke this system call in the global initialization routine.

In addition, this change is beneficial because it:

1. Avoids extra macOS system calls for every IP lookup.
2. Consolidates macOS-specific initialization in a separate file.

Fixes curl#11252
Closes curl#11254
@stanhu stanhu mentioned this pull request Jan 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bagder bagder bagder approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

lib/hostip.c: SCDynamicStoreCopyProxies is not safe to run in a fork
3 participants
@stanhu @zajdee @bagder