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

runtime: macOS syscall.Exec can get SIGILL due to preemption signal #41702

Closed
ianlancetaylor opened this issue Sep 30, 2020 · 16 comments
Closed

runtime: macOS syscall.Exec can get SIGILL due to preemption signal #41702

ianlancetaylor opened this issue Sep 30, 2020 · 16 comments

Comments

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 30, 2020

On macOS 10.14 and 10.15 (at least) a Go program calling syscall.Exec can fail due to SIGILL in the newly execed image because of preemption signals.

This is a bug in macOS. Here is a C program that demonstrates the problem. This program will occasionally fail with SIGILL.

For Go, we should have a workaround, which means in syscall.Exec we should disable preemption via signal. We already have hooks that we can use for this (syscall.runtime_BeforeExec and syscall.runtime_AfterExec).

#include <sys/errno.h>
#include <assert.h>
#include <pthread.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
 
extern char **environ;
 
static pthread_t sleep_thread;
 
void sighandler(int signal) {}
 
void* signal_thread_main(void* unused_args) {
  while (1) {
    int ret = pthread_kill(sleep_thread, SIGURG);
    assert(ret == 0);
  }
  return NULL;
}
 
void* exec_thread_main(void* unused_args) {
  char* exec_args[] = {"/usr/bin/python", "--version", NULL};
  int ret;
 
  ret = execve(exec_args[0], exec_args, environ);
  assert(ret == 0);
  return NULL;
}
 
void* sleep_thread_main(void* unused_args) {
  unsigned int to_sleep = 60;
  while (to_sleep > 0) {
    to_sleep = sleep(to_sleep);
  }
  return NULL;
}
 
int main(int argc, char* argv[]) {
  pthread_t signal_thread, exec_thread;
  int ret;
 
  signal(SIGURG, &sighandler);
 
  ret = pthread_create(&sleep_thread, NULL, &sleep_thread_main, NULL);
  assert(ret == 0);
 
  ret = pthread_create(&signal_thread, NULL, &signal_thread_main, NULL);
  assert(ret == 0);
 
  sleep(1);  // Give signalling time to start                             
  ret = pthread_create(&exec_thread, NULL, &exec_thread_main, NULL);
  assert(ret == 0);
 
  sleep(30);
}
@ianlancetaylor
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Sep 30, 2020

@gopherbot Please open backport issues

This issue can cause any Go program that uses syscall.Exec on macOS to crash during the exec. We should backport the fix (when we have it) so that this will work for older releases.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 30, 2020

Backport issue(s) opened: #41703 (for 1.14), #41704 (for 1.15).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

Loading

@ianlancetaylor
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Oct 8, 2020

FYI, a bug was reportedly filed with Apple at https://feedbackassistant.apple.com/feedback/8759414 (but I can't see that report because I don't have an Apple ID).

Loading

@randall77
Copy link
Contributor

@randall77 randall77 commented Oct 8, 2020

I have an Apple ID and I can't see it.
Feedback assistant seems to make things private by default. There are only 15 bugs I have access to, and that can't be all the bugs in existence.

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Oct 8, 2020

I have access to the Google Apple account from which the report was filed (obtained for #33041 (comment)) and can confirm that that feedback report describes this issue.

Loading

@ianlancetaylor ianlancetaylor self-assigned this Oct 14, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 14, 2020

Change https://golang.org/cl/262438 mentions this issue: runtime: stop preemption during syscall.Exec on Darwin

Loading

@gopherbot gopherbot closed this in 64fb6ae Oct 15, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 15, 2020

Change https://golang.org/cl/262717 mentions this issue: [release-branch.go1.15] runtime: stop preemption during syscall.Exec on Darwin

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 15, 2020

Change https://golang.org/cl/262737 mentions this issue: [release-branch.go1.14] runtime: stop preemption during syscall.Exec on Darwin

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 15, 2020

Change https://golang.org/cl/262738 mentions this issue: syscall: use MustHaveExec in TestExec

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 15, 2020

Change https://golang.org/cl/262817 mentions this issue: runtime: wait for preemption signals before syscall.Exec

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Oct 16, 2020

Reopening until CL 262817 is submitted. (Test is failing right now.)

Loading

@rsc rsc reopened this Oct 16, 2020
@gopherbot gopherbot closed this in 05739d6 Oct 16, 2020
gopherbot pushed a commit that referenced this issue Oct 17, 2020
For #41702

Change-Id: Ib2b15e52aa1fef2f5e644b316c726150252fa9f8
Reviewed-on: https://go-review.googlesource.com/c/go/+/262738
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Oct 20, 2020
…on Darwin

On current macOS versions a program that receives a signal during an
execve can fail with a SIGILL signal. This appears to be a macOS
kernel bug. It has been reported to Apple.

This CL partially works around the problem by using execLock to not
send preemption signals during execve. Of course some other stray
signal could occur, but at least we can avoid exacerbating the problem.
We can't simply disable signals, as that would mean that the exec'ed
process would start with all signals blocked, which it likely does not
expect.

For #41702
Fixes #41703

Change-Id: I91b0add967b315671ddcf73269c4d30136e579b4
Reviewed-on: https://go-review.googlesource.com/c/go/+/262438
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
(cherry picked from commit 64fb6ae)
Reviewed-on: https://go-review.googlesource.com/c/go/+/262737
gopherbot pushed a commit that referenced this issue Oct 20, 2020
…on Darwin

On current macOS versions a program that receives a signal during an
execve can fail with a SIGILL signal. This appears to be a macOS
kernel bug. It has been reported to Apple.

This CL partially works around the problem by using execLock to not
send preemption signals during execve. Of course some other stray
signal could occur, but at least we can avoid exacerbating the problem.
We can't simply disable signals, as that would mean that the exec'ed
process would start with all signals blocked, which it likely does not
expect.

For #41702
Fixes #41704

Change-Id: I91b0add967b315671ddcf73269c4d30136e579b4
Reviewed-on: https://go-review.googlesource.com/c/go/+/262438
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
(cherry picked from commit 64fb6ae)
Reviewed-on: https://go-review.googlesource.com/c/go/+/262717
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 20, 2020

Change https://golang.org/cl/264020 mentions this issue: [possible] syscall: use MustHaveExec in TestExec

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 20, 2020

Change https://golang.org/cl/264021 mentions this issue: [release-branch.go1.14] syscall: use MustHaveExec in TestExec

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 20, 2020

Change https://golang.org/cl/264022 mentions this issue: [release-branch.go1.15] runtime: wait for preemption signals before syscall.Exec

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 20, 2020

Change https://golang.org/cl/264023 mentions this issue: [release-branch.go1.14] runtime: wait for preemption signals before syscall.Exec

Loading

gopherbot pushed a commit that referenced this issue Oct 20, 2020
For #41702
For #41703

Change-Id: Ib2b15e52aa1fef2f5e644b316c726150252fa9f8
Reviewed-on: https://go-review.googlesource.com/c/go/+/262738
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
(cherry picked from commit 11cfb48)
Reviewed-on: https://go-review.googlesource.com/c/go/+/264021
gopherbot pushed a commit that referenced this issue Oct 20, 2020
For #41702
For #41704

Change-Id: Ib2b15e52aa1fef2f5e644b316c726150252fa9f8
Reviewed-on: https://go-review.googlesource.com/c/go/+/262738
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
(cherry picked from commit 11cfb48)
Reviewed-on: https://go-review.googlesource.com/c/go/+/264020
gopherbot pushed a commit that referenced this issue Oct 20, 2020
…yscall.Exec

For #41702
For #41704
For #42023

Change-Id: If07f40b1d73b8f276ee28ffb8b7214175e56c24d
Reviewed-on: https://go-review.googlesource.com/c/go/+/262817
Trust: Ian Lance Taylor <iant@golang.org>
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
(cherry picked from commit 05739d6)
Reviewed-on: https://go-review.googlesource.com/c/go/+/264022
gopherbot pushed a commit that referenced this issue Oct 20, 2020
…yscall.Exec

For #41702
For #41703
For #42023

Change-Id: If07f40b1d73b8f276ee28ffb8b7214175e56c24d
Reviewed-on: https://go-review.googlesource.com/c/go/+/262817
Trust: Ian Lance Taylor <iant@golang.org>
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
(cherry picked from commit 05739d6)
Reviewed-on: https://go-review.googlesource.com/c/go/+/264023
claucece added a commit to claucece/go that referenced this issue Oct 22, 2020
…on Darwin

On current macOS versions a program that receives a signal during an
execve can fail with a SIGILL signal. This appears to be a macOS
kernel bug. It has been reported to Apple.

This CL partially works around the problem by using execLock to not
send preemption signals during execve. Of course some other stray
signal could occur, but at least we can avoid exacerbating the problem.
We can't simply disable signals, as that would mean that the exec'ed
process would start with all signals blocked, which it likely does not
expect.

For golang#41702
Fixes golang#41704

Change-Id: I91b0add967b315671ddcf73269c4d30136e579b4
Reviewed-on: https://go-review.googlesource.com/c/go/+/262438
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
(cherry picked from commit 64fb6ae)
Reviewed-on: https://go-review.googlesource.com/c/go/+/262717
claucece added a commit to claucece/go that referenced this issue Oct 22, 2020
For golang#41702
For golang#41704

Change-Id: Ib2b15e52aa1fef2f5e644b316c726150252fa9f8
Reviewed-on: https://go-review.googlesource.com/c/go/+/262738
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
(cherry picked from commit 11cfb48)
Reviewed-on: https://go-review.googlesource.com/c/go/+/264020
claucece added a commit to claucece/go that referenced this issue Oct 22, 2020
…yscall.Exec

For golang#41702
For golang#41704
For golang#42023

Change-Id: If07f40b1d73b8f276ee28ffb8b7214175e56c24d
Reviewed-on: https://go-review.googlesource.com/c/go/+/262817
Trust: Ian Lance Taylor <iant@golang.org>
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
(cherry picked from commit 05739d6)
Reviewed-on: https://go-review.googlesource.com/c/go/+/264022
@gopherbot
Copy link

@gopherbot gopherbot commented Dec 3, 2020

Change https://golang.org/cl/275293 mentions this issue: runtime: avoid receiving preemotion signal while exec'ing

Loading

gopherbot pushed a commit that referenced this issue Dec 4, 2020
The iOS kernel has the same problem as the macOS kernel. Extend
the workaround of #41702 (CL 262438 and CL 262817) to iOS.

Updates #35851.

Change-Id: I7ccec00dc96643c08c5be8b385394856d0fa0f64
Reviewed-on: https://go-review.googlesource.com/c/go/+/275293
Trust: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Dec 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants