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

Fix an assertion failure at exit in the macOS app sandbox #33944

Closed
wants to merge 5 commits into from
Closed

Fix an assertion failure at exit in the macOS app sandbox #33944

wants to merge 5 commits into from

Conversation

branchseer
Copy link
Contributor

The node process raises at exit an assertion failure at

node/src/node.cc

Line 740 in a85ce88

CHECK_EQ(0, err);
when it runs in macOS app sandbox.

This PR includes

  • A basic test that verifies Node.js is able to run in the macOS app sandbox,
  • A fix of the problem described above,
  • A test of the fix.

Checklist

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@branchseer
src: tolerate EPERM returned from tcsetattr
317621b 
macOS app sandbox makes tcsetattr return EPERM. The CHECK_EQ(0, err) here would fail when a sandboxed Node.js process is exiting. This commit fixes this issue.
@branchseer
test: add test for running in macOS app sandbox
5f4f969 
Bare-bone command-line executables cannot run directly in the app sandbox. To test that Node.js is able to run in the sandbox (and to test the fix in 317621b), this commit creates a typical Cocoa app bundle, puts the node executable in it and calles Apple's codesign command to enable sandbox.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 18, 2020
@richardlau
Copy link
Member

This looks okay to me, but I'm not really a macOS person.

cc @nodejs/platform-macos

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 25, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/32076/

jasnell pushed a commit that referenced this pull request Jun 25, 2020
@branchseer @jasnell
src: tolerate EPERM returned from tcsetattr
cb2c810 
macOS app sandbox makes tcsetattr return EPERM. The CHECK_EQ(0, err)
here would fail when a sandboxed Node.js process is exiting. This commit
fixes this issue.

* test: add test for running in macOS app sandbox

Bare-bone command-line executables cannot run directly in the app sandbox.
To test that Node.js is able to run in the sandbox (and to test the fix in
317621b), this commit creates a typical
Cocoa app bundle, puts the node executable in it and calles Apple's codesign
command to enable sandbox.

* test: use process.execPath to get path of testing node

PR-URL: #33944
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jun 25, 2020

Landed in cb2c810

@jasnell jasnell closed this Jun 25, 2020
codebytere pushed a commit that referenced this pull request Jun 27, 2020
@branchseer @codebytere
src: tolerate EPERM returned from tcsetattr
da81930 
macOS app sandbox makes tcsetattr return EPERM. The CHECK_EQ(0, err)
here would fail when a sandboxed Node.js process is exiting. This commit
fixes this issue.

* test: add test for running in macOS app sandbox

Bare-bone command-line executables cannot run directly in the app sandbox.
To test that Node.js is able to run in the sandbox (and to test the fix in
317621b), this commit creates a typical
Cocoa app bundle, puts the node executable in it and calles Apple's codesign
command to enable sandbox.

* test: use process.execPath to get path of testing node

PR-URL: #33944
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Jun 28, 2020
Merged
codebytere pushed a commit that referenced this pull request Jun 30, 2020
@branchseer @codebytere
src: tolerate EPERM returned from tcsetattr
2510ffc 
macOS app sandbox makes tcsetattr return EPERM. The CHECK_EQ(0, err)
here would fail when a sandboxed Node.js process is exiting. This commit
fixes this issue.

* test: add test for running in macOS app sandbox

Bare-bone command-line executables cannot run directly in the app sandbox.
To test that Node.js is able to run in the sandbox (and to test the fix in
317621b), this commit creates a typical
Cocoa app bundle, puts the node executable in it and calles Apple's codesign
command to enable sandbox.

* test: use process.execPath to get path of testing node

PR-URL: #33944
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Jul 10, 2020
@branchseer @codebytere
src: tolerate EPERM returned from tcsetattr
46da871 
macOS app sandbox makes tcsetattr return EPERM. The CHECK_EQ(0, err)
here would fail when a sandboxed Node.js process is exiting. This commit
fixes this issue.

* test: add test for running in macOS app sandbox

Bare-bone command-line executables cannot run directly in the app sandbox.
To test that Node.js is able to run in the sandbox (and to test the fix in
317621b), this commit creates a typical
Cocoa app bundle, puts the node executable in it and calles Apple's codesign
command to enable sandbox.

* test: use process.execPath to get path of testing node

PR-URL: #33944
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Jul 12, 2020
@branchseer @codebytere
src: tolerate EPERM returned from tcsetattr
b57778f 
macOS app sandbox makes tcsetattr return EPERM. The CHECK_EQ(0, err)
here would fail when a sandboxed Node.js process is exiting. This commit
fixes this issue.

* test: add test for running in macOS app sandbox

Bare-bone command-line executables cannot run directly in the app sandbox.
To test that Node.js is able to run in the sandbox (and to test the fix in
317621b), this commit creates a typical
Cocoa app bundle, puts the node executable in it and calles Apple's codesign
command to enable sandbox.

* test: use process.execPath to get path of testing node

PR-URL: #33944
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Jul 13, 2020
Merged
codebytere pushed a commit that referenced this pull request Jul 14, 2020
@branchseer @codebytere
src: tolerate EPERM returned from tcsetattr
d16e51b 
macOS app sandbox makes tcsetattr return EPERM. The CHECK_EQ(0, err)
here would fail when a sandboxed Node.js process is exiting. This commit
fixes this issue.

* test: add test for running in macOS app sandbox

Bare-bone command-line executables cannot run directly in the app sandbox.
To test that Node.js is able to run in the sandbox (and to test the fix in
317621b), this commit creates a typical
Cocoa app bundle, puts the node executable in it and calles Apple's codesign
command to enable sandbox.

* test: use process.execPath to get path of testing node

PR-URL: #33944
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

This test seems to be reliably failing in GitHub actions

@richardlau
Copy link
Member

This test seems to be reliably failing in GitHub actions

There's #34331.

cjihrig added a commit to cjihrig/node that referenced this pull request Jul 22, 2020
@cjihrig
test: force resigning of app
e7ed066 
In the GitHub Actions CI, test-macos-app-sandbox.js can fail due
to the application already being signed. This commit updates
the test to handle that condition.

Refs: nodejs#33944
PR-URL: nodejs#34331
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
cjihrig added a commit that referenced this pull request Jul 23, 2020
@cjihrig
test: force resigning of app
eba2ae4 
In the GitHub Actions CI, test-macos-app-sandbox.js can fail due
to the application already being signed. This commit updates
the test to handle that condition.

Refs: #33944
PR-URL: #34331
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request Jul 27, 2020
@cjihrig @MylesBorins
test: force resigning of app
cd35d00 
In the GitHub Actions CI, test-macos-app-sandbox.js can fail due
to the application already being signed. This commit updates
the test to handle that condition.

Refs: #33944
PR-URL: #34331
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@cjihrig @addaleax
test: force resigning of app
771c3e4 
In the GitHub Actions CI, test-macos-app-sandbox.js can fail due
to the application already being signed. This commit updates
the test to handle that condition.

Refs: #33944
PR-URL: #34331
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@cjihrig @addaleax
test: force resigning of app
26c5f9f 
In the GitHub Actions CI, test-macos-app-sandbox.js can fail due
to the application already being signed. This commit updates
the test to handle that condition.

Refs: #33944
PR-URL: #34331
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau left review comments

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@branchseer @richardlau @nodejs-github-bot @jasnell @MylesBorins @addaleax