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: crash in sandbox on linux when getting execPath #15701

Merged
merged 4 commits into from Nov 15, 2018

Conversation

Projects
None yet
3 participants
@nornagon
Copy link
Contributor

commented Nov 13, 2018

calling base::PathService::Get(base::FILE_EXE, &path) on linux crashes with --enable-sandbox:

#0  0x00007faabaa2ae97 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007faabaa2c801 in __GI_abort () at abort.c:79
#2  0x000056153ea42085 in  () at ./../../base/debug/debugger_posix.cc:258
#3  0x000056153e97e5be in ~LogMessage() () at ./../../base/logging.cc:858
#4  0x000056153ea55ac4 in PathProviderPosix() () at ./../../base/base_paths_posix.cc:43
#5  0x000056153e9b11ed in Get() () at ./../../base/path_service.cc:207
#6  0x000056153e90857c in GetExecPath () at ../../electron/atom/renderer/atom_sandboxed_renderer_client.cc:85
#7  0x000056153e90857c in InitializeBindings() () at ../../electron/atom/renderer/atom_sandboxed_renderer_client.cc:160
#8  0x000056153e9093f5 in DidCreateScriptContext() () at ../../electron/atom/renderer/atom_sandboxed_renderer_client.cc:219
#9  0x000056153e906607 in DidCreateScriptContext() () at ../../electron/atom/renderer/atom_render_frame_observer.cc:96
#10 0x0000561542469ae5 in DidCreateScriptContext() () at ./../../content/renderer/render_frame_impl.cc:5150

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • tests are changed or added
  • relevant documentation is changed or added
  • PR title follows semantic commit guidelines

Release Notes

Notes:

  • fix a crash on Linux when starting a sandboxed renderer
  • the resourcesPath property is no longer available on process in sandboxed renderers
fix: remove execPath and resourcesPath from sandbox process
calling base::PathService::Get(base::FILE_EXE, &path) on linux crashes in sandbox.

@nornagon nornagon requested review from deepak1556, miniak and Nov 13, 2018

@nornagon nornagon requested a review from as a code owner Nov 13, 2018

@nornagon

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2018

Hmm, looks like execPath is used for determining whether security warnings should be logged. We could probably find a different way to do that.

@miniak could you explain why execPath was changed in #13839?

@nornagon nornagon changed the title fix: remove execPath and resourcesPath from sandbox process fix: crash in sandbox on linux when getting execPath Nov 13, 2018

@miniak

miniak approved these changes Nov 14, 2018

@miniak

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

@nornagon, I don’t remember exactly, I should probably just have used process.helperExecPath in the first place

@nornagon

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

@miniak it's still unclear to me what #13839 was fixing, and additionally unclear why you would ever want the path to Electron Helper rather than to Electron.

@nornagon

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

I'm also still uncertain about removing resourcesPath. Can anyone say for sure whether that will/won't break something important?

@miniak

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

@nornagon resourcesPath was added just for consistency. We don’t use it. But somebody else might.

@nornagon

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2018

Merging this without review from @electron/docs because the change to docs is trivial.

I'm removing resourcesPath because I'm not sure that a sandboxed renderer could actually do anything useful with that path, what with being sandboxed and all. Happy to restore it if someone comes up with a use case though!

@nornagon nornagon merged commit 0642be2 into master Nov 15, 2018

25 of 27 checks passed

electron-mas-testing Build #20181114.17 has failed
Details
electron-osx-testing Build #20181114.18 has failed
Details
Absolute Zero
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Legacy commit status override — see details
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
electron-arm-testing Build #20181113.18 succeeded
Details
electron-arm64-testing Build #20181113.18 succeeded
Details
electron-lint Build #20181114.11 succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Nov 15, 2018

Release Notes Persisted

  • fix a crash on Linux when starting a sandboxed renderer
  • the resourcesPath property is no longer available on process in sandboxed renderers

@nornagon nornagon deleted the sandbox-execpath branch Nov 15, 2018

neo291 added a commit to neo291/electron that referenced this pull request Nov 16, 2018

neo291 added a commit to neo291/electron that referenced this pull request Nov 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.