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

std: Synchronize access to global env during `exec` #55939

Merged
merged 2 commits into from Nov 15, 2018

Conversation

Projects
None yet
7 participants
@alexcrichton
Member

alexcrichton commented Nov 13, 2018

This commit, after reverting #55359, applies a different fix for #46775
while also fixing #55775. The basic idea was to go back to pre-#55359
libstd, and then fix #46775 in a way that doesn't expose #55775.

The issue described in #46775 boils down to two problems:

  • First, the global environment is reset during exec but, but if the
    exec call fails then the global environment was a dangling pointer
    into free'd memory as the block of memory was deallocated when
    Command is dropped. This is fixed in this commit by installing a
    Drop stack object which ensures that the environ pointer is
    preserved on a failing exec.

  • Second, the global environment was accessed in an unsynchronized
    fashion during exec. This was fixed by ensuring that the
    Rust-specific environment lock is acquired for these system-level
    operations.

Thanks to Alex Gaynor for pioneering the solution here!

Closes #55775

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 13, 2018

@@ -84,4 +94,14 @@ fn main() {
assert!(output.status.success());
assert!(output.stderr.is_empty());
assert_eq!(output.stdout, b"passed\n");
let output = Command::new(&me).arg("exec-test6").output().unwrap();

This comment has been minimized.

@sfackler

sfackler Nov 13, 2018

Member

Are exec-test6 and exec-test7 identical?

This comment has been minimized.

@alexcrichton

alexcrichton Nov 13, 2018

Member

I think morally yeah, they're just testing the two routes you can get at removing PATH from env (either by clearing or singularly removing it)

This comment has been minimized.

@sfackler

sfackler Nov 13, 2018

Member

Oh I missed the path manipulation up above.

@sfackler

This comment has been minimized.

Member

sfackler commented Nov 13, 2018

It's unfortunate we can't use execvpe for this. We could potentially use it if supported and the command hasn't overridden PATH? Doesn't seem like we need to block this on that though.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 13, 2018

I think even with execvpe we'd still need a large degree of what's going on here, specifically the environment lock. If the command didn't override PATH then we could use execvpe today yeah, but I think it's actually always been a bug that we don't call the exec* family of functions with the global env lock.

@sfackler

This comment has been minimized.

Member

sfackler commented Nov 13, 2018

Multithreaded exec considered harmful :(

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Nov 13, 2018

📌 Commit b967d55 has been approved by sfackler

@bors

This comment has been minimized.

Contributor

bors commented Nov 13, 2018

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #55912
@bors

This comment has been minimized.

Contributor

bors commented Nov 13, 2018

📌 Commit b967d55 has been approved by sfackler

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 13, 2018

@bors: p=5

we'll need to backport this to fix the beta-regression as well

@bors

This comment has been minimized.

Contributor

bors commented Nov 14, 2018

⌛️ Testing commit b967d55 with merge 47f5aea...

bors added a commit that referenced this pull request Nov 14, 2018

Auto merge of #55939 - alexcrichton:path-regression-again, r=sfackler
std: Synchronize access to global env during `exec`

This commit, after reverting #55359, applies a different fix for #46775
while also fixing #55775. The basic idea was to go back to pre-#55359
libstd, and then fix #46775 in a way that doesn't expose #55775.

The issue described in #46775 boils down to two problems:

* First, the global environment is reset during `exec` but, but if the
  `exec` call fails then the global environment was a dangling pointer
  into free'd memory as the block of memory was deallocated when
  `Command` is dropped. This is fixed in this commit by installing a
  `Drop` stack object which ensures that the `environ` pointer is
  preserved on a failing `exec`.

* Second, the global environment was accessed in an unsynchronized
  fashion during `exec`. This was fixed by ensuring that the
  Rust-specific environment lock is acquired for these system-level
  operations.

Thanks to Alex Gaynor for pioneering the solution here!

Closes #55775
@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 14, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:228cb124:start=1542152451577409021,finish=1542152453840676678,duration=2263267657
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading https://files.pythonhosted.org/packages/ab/4e/ae2cf04ce20a86790605a67c0a5010df912625dc826a3b1c82ea71746668/awscli-1.16.54-py2.py3-none-any.whl (1.4MB)
    0% |▎                               | 10kB 4.2MB/s eta 0:00:01
    1% |▌                               | 20kB 1.7MB/s eta 0:00:01
    2% |▊                               | 30kB 2.0MB/s eta 0:00:01
    2% |█                               | 40kB 1.8MB/s eta 0:00:01
---
[00:50:18] .................................................................................................... 100/5017
[00:50:21] .................................................................................................... 200/5017
[00:50:23] .............................ii............................................ii...................ii.. 300/5017
[00:50:26] ..............................................................................................iii... 400/5017
[00:50:29] .....iiiiiiii.iii............................iii...........................................i........ 500/5017
[00:50:36] .................................................................................................... 700/5017
[00:50:42] .................................................................................i...........i...... 800/5017
[00:50:46] .................................................................................................... 900/5017
[00:50:49] iiiii..................ii.iiii...................................................................... 1000/5017
---
[00:51:25] .................................................................................................... 2200/5017
[00:51:29] .................................................................................................... 2300/5017
[00:51:33] .................................................................................................... 2400/5017
[00:51:37] .................................................................................................... 2500/5017
[00:51:40] .................................................................................iiiiiiiii.......... 2600/5017
[00:51:47] ..............................................ii.................................................... 2800/5017
[00:51:50] .................................................................................................... 2900/5017
[00:51:54] .................................................................................................... 3000/5017
[00:51:57] ........................................i........................................................... 3100/5017
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:05:27] 
[01:05:27] running 116 tests
[01:05:30] i..ii...iii..iiii.....i...i.........i..iii...........i.....i.....ii...i..i.ii..............i...ii..i 100/116
[01:05:31] i.i....iiii.....
[01:05:31] 
[01:05:31]  finished in 3.432
[01:05:31] travis_fold:end:test_codegen

---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:05:45] 
[01:05:45] running 118 tests
[01:06:09] .iiiii...i.....i..i...i..i.i..i.i..i.....i..i....i..........iiii.........i.i....i...i.......ii.i.i.i 100/118
[01:06:13] ......iii.i.....ii
[01:06:13] 
[01:06:13]  finished in 28.461
[01:06:13] travis_fold:end:test_debuginfo

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors

This comment has been minimized.

Contributor

bors commented Nov 14, 2018

💔 Test failed - status-travis

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 14, 2018

The job arm-android of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading https://files.pythonhosted.org/packages/ab/4e/ae2cf04ce20a86790605a67c0a5010df912625dc826a3b1c82ea71746668/awscli-1.16.54-py2.py3-none-any.whl (1.4MB)
    0% |▎                               | 10kB 10.9MB/s eta 0:00:01
    1% |▌                               | 20kB 1.9MB/s eta 0:00:01
    2% |▊                               | 30kB 2.2MB/s eta 0:00:01
    2% |█                               | 40kB 2.0MB/s eta 0:00:01
---
[01:23:13] ---- [run-pass] run-pass/command-exec.rs stdout ----
[01:23:13] 
[01:23:13] error: test run failed!
[01:23:13] status: exit code: 101
[01:23:13] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/remote-test-client" "run" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/command-exec/a"
[01:23:13] ------------------------------------------
[01:23:13] uploaded "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/command-exec/a", waiting for result
[01:23:13] 
[01:23:13] ------------------------------------------
[01:23:13] ------------------------------------------
[01:23:13] stderr:
[01:23:13] ------------------------------------------
[01:23:13] thread 'main' panicked at 'assertion failed: output.status.success()', /checkout/src/test/run-pass/command-exec.rs:99:5
[01:23:13] 
[01:23:13] ------------------------------------------
[01:23:13] 
[01:23:13] thread '[run-pass] run-pass/command-exec.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3282:9
---
[01:23:13] 
[01:23:13] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:503:22
[01:23:13] 
[01:23:13] 
[01:23:13] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/arm-linux-androideabi/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/run-pass" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "--stage-id" "stage2-arm-linux-androideabi" "--mode" "run-pass" "--target" "arm-linux-androideabi" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--linker" "/android/ndk/arm-14/bin/arm-linux-androideabi-clang" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/arm-linux-androideabi/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--llvm-version" "8.0.0svn\n" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--remote-test-client" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/remote-test-client" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "/android/ndk/arm-14" "--color" "always"
[01:23:13] 
[01:23:13] 
[01:23:13] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --target arm-linux-androideabi
[01:23:13] Build completed unsuccessfully in 1:11:14
---
travis_time:end:056cab44:start=1542163449675642587,finish=1542163449694083174,duration=18440587
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:00532b1b
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:2b26fcf0
travis_time:start:2b26fcf0
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:2a84b523
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@alexcrichton alexcrichton force-pushed the alexcrichton:path-regression-again branch from b967d55 to 95585f4 Nov 14, 2018

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 14, 2018

@bors: r=sfackler

@bors

This comment has been minimized.

Contributor

bors commented Nov 14, 2018

📌 Commit 95585f4 has been approved by sfackler

@kennytm

This comment has been minimized.

Member

kennytm commented Nov 14, 2018

@bors p=83

rollup fairness

@bors

This comment has been minimized.

Contributor

bors commented Nov 14, 2018

⌛️ Testing commit 95585f4 with merge d3cdb68...

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 14, 2018

@bors: retry

@bors

This comment has been minimized.

Contributor

bors commented Nov 14, 2018

⌛️ Testing commit 95585f4 with merge 07f4065...

bors added a commit that referenced this pull request Nov 14, 2018

Auto merge of #55939 - alexcrichton:path-regression-again, r=sfackler
std: Synchronize access to global env during `exec`

This commit, after reverting #55359, applies a different fix for #46775
while also fixing #55775. The basic idea was to go back to pre-#55359
libstd, and then fix #46775 in a way that doesn't expose #55775.

The issue described in #46775 boils down to two problems:

* First, the global environment is reset during `exec` but, but if the
  `exec` call fails then the global environment was a dangling pointer
  into free'd memory as the block of memory was deallocated when
  `Command` is dropped. This is fixed in this commit by installing a
  `Drop` stack object which ensures that the `environ` pointer is
  preserved on a failing `exec`.

* Second, the global environment was accessed in an unsynchronized
  fashion during `exec`. This was fixed by ensuring that the
  Rust-specific environment lock is acquired for these system-level
  operations.

Thanks to Alex Gaynor for pioneering the solution here!

Closes #55775
@bors

This comment has been minimized.

Contributor

bors commented Nov 14, 2018

💔 Test failed - status-travis

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 14, 2018

The job x86_64-gnu-tools of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:54:02] test /checkout/src/doc/book/2018-edition/src/ch10-02-traits.md - _::Traits__Defining_Shared_Behavior::Using_Trait_Bounds_to_Conditionally_Implement_Methods (line 568) ... ignored
[00:54:02] test /checkout/src/doc/book/2018-edition/src/ch10-02-traits.md - _::Traits__Defining_Shared_Behavior::Fixing_the_ (line 477) ... ok
[00:54:02] test /checkout/src/doc/book/2018-edition/src/ch10-02-traits.md - _::Traits__Defining_Shared_Behavior::Using_Trait_Bounds_to_Conditionally_Implement_Methods (line 530) ... ok
[00:54:02] test /checkout/src/doc/book/2018-edition/src/ch10-02-traits.md - _::Traits__Defining_Shared_Behavior::Using_Trait_Bounds_to_Conditionally_Implement_Methods (line 579) ... ok
[00:55:02] test /checkout/src/doc/book/2018-edition/src/ch10-02-traits.md - _::Traits__Defining_Shared_Behavior::Implementing_a_Trait_on_a_Type (line 67) ... test /checkout/src/doc/book/2018-edition/src/ch10-02-traits.md - _::Traits__Defining_Shared_Behavior::Implementing_a_Trait_on_a_Type (line 67) has been running for over 60 seconds
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@kennytm

This comment has been minimized.

Member

kennytm commented Nov 14, 2018

🤔 Any chance this change causes dead-lock during doc tests?

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 14, 2018

Yes this seems non spurious, I will investigate

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 14, 2018

Oh right, if a process forks with the lock held in one thread we can't then try to acquire it later. This will.need to acquire the lock before the fork

std: Synchronize access to global env during `exec`
This commit, after reverting #55359, applies a different fix for #46775
while also fixing #55775. The basic idea was to go back to pre-#55359
libstd, and then fix #46775 in a way that doesn't expose #55775.

The issue described in #46775 boils down to two problems:

* First, the global environment is reset during `exec` but, but if the
  `exec` call fails then the global environment was a dangling pointer
  into free'd memory as the block of memory was deallocated when
  `Command` is dropped. This is fixed in this commit by installing a
  `Drop` stack object which ensures that the `environ` pointer is
  preserved on a failing `exec`.

* Second, the global environment was accessed in an unsynchronized
  fashion during `exec`. This was fixed by ensuring that the
  Rust-specific environment lock is acquired for these system-level
  operations.

Thanks to Alex Gaynor for pioneering the solution here!

Closes #55775

Co-authored-by: Alex Gaynor <alex.gaynor@gmail.com>

@alexcrichton alexcrichton force-pushed the alexcrichton:path-regression-again branch from 95585f4 to 4032b7a Nov 14, 2018

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 14, 2018

@bors: r=sfackler

@bors

This comment has been minimized.

Contributor

bors commented Nov 14, 2018

📌 Commit 4032b7a has been approved by sfackler

@bors

This comment has been minimized.

Contributor

bors commented Nov 14, 2018

⌛️ Testing commit 4032b7a with merge 7d3b9b1...

bors added a commit that referenced this pull request Nov 14, 2018

Auto merge of #55939 - alexcrichton:path-regression-again, r=sfackler
std: Synchronize access to global env during `exec`

This commit, after reverting #55359, applies a different fix for #46775
while also fixing #55775. The basic idea was to go back to pre-#55359
libstd, and then fix #46775 in a way that doesn't expose #55775.

The issue described in #46775 boils down to two problems:

* First, the global environment is reset during `exec` but, but if the
  `exec` call fails then the global environment was a dangling pointer
  into free'd memory as the block of memory was deallocated when
  `Command` is dropped. This is fixed in this commit by installing a
  `Drop` stack object which ensures that the `environ` pointer is
  preserved on a failing `exec`.

* Second, the global environment was accessed in an unsynchronized
  fashion during `exec`. This was fixed by ensuring that the
  Rust-specific environment lock is acquired for these system-level
  operations.

Thanks to Alex Gaynor for pioneering the solution here!

Closes #55775
@bors

This comment has been minimized.

Contributor

bors commented Nov 15, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: sfackler
Pushing 7d3b9b1 to master...

@bors bors merged commit 4032b7a into rust-lang:master Nov 15, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@alexcrichton alexcrichton deleted the alexcrichton:path-regression-again branch Nov 15, 2018

bors added a commit that referenced this pull request Nov 15, 2018

Auto merge of #55978 - alexcrichton:beta-next, r=Mark-Simulacrum
[beta] std: Synchronize access to global env during `exec`

This is a beta backport of #55939
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment