rustc: Try to contain prepends to PATH #34083
Conversation
|
r? @nrc (rust_highfive has picked a reviewer for you, use r? to override) |
|
Would some kind of regression test be appreciated? I mean something like a crate with 1500 or so very simple doctests, which shouldn't take a very long time to execute even on Windows, and for which the pass condition is no spawn errors being thrown. Using some probability theory we could choose the exact number of dummy cases to give us any degree of confidence of such race conditions happening for a certain number of times, but I think 2000 at most would be enough. |
|
Perhaps yeah! I don't have the ability to create a test right now and verify that it actually fails |
| let mut new_path = sess.host_filesearch(PathKind::All) | ||
| .get_dylib_search_paths(); | ||
| new_path.extend(env::split_paths(&_old_path)); | ||
| new_path.retain(|p| !old_paths.contains(p)); |
There was a problem hiding this comment.
This changes the semantics! Before we're ensuring that the intended paths are always searched first, but now one only have to guess the path in advance to prevent it from being prepended. This might have security consequences given Windows's reputation on dynamic library searching, and IMO it'll be better to delete the path from old_path instead.
There was a problem hiding this comment.
Yeah I don't think it actually matters, but the hash set is probably overkill here anyway.
|
@alexcrichton Okay, I'll attempt to do this EDIT: no this perhaps shouldn't count as a testcase, because race conditions are inherently difficult and sporadic things to test, and in case someone runs the test with a single core they would simply be wasting their time. I would paste my results as a gist later maybe. |
This commit attempts to bring our prepends to PATH on Windows when loading plugins because we've been seeing quite a few issues with failing to spawn a process on Windows, the leading theory of which is that PATH is too large as a result of this. Currently this is mostly a stab in the dark as it's not confirmed to actually fix the problem, but it's probably not a bad change to have anyway! cc rust-lang#33844 Closes rust-lang#17360
| let mut new_path = sess.host_filesearch(PathKind::All) | ||
| .get_dylib_search_paths(); | ||
| new_path.extend(env::split_paths(&_old_path)); | ||
| for path in env::split_paths(&old_path) { | ||
| if !new_path.contains(&path) { |
There was a problem hiding this comment.
Although this makes the for loop O(n^2), it is fine IMO because new_path shouldn't have too many entries.
There was a problem hiding this comment.
No, it's repeatedly adding up!
There was a problem hiding this comment.
I would do something like:
let old_path = env::split_paths(&old_path).filter(|p| !new_path.contains(p)).collect();
new_path.extend(old_path);This collect is necessary to make sure all new_path queries are looking at its pristine state. Or make another HashSet from new_path just to be explicit and O(1), but I'd not go that far. (Consulting a constant list is also O(1) actually.)
There was a problem hiding this comment.
No, when run concurrently everything is adding the same paths, so it doesn't add up. There's no way this is more expensive than, say, reading the source file.
There was a problem hiding this comment.
Fair point! Premature optimization is evil, yeah? 😈
|
LGTM (or, better than it was, at least :). If you care about the remaining raciness: it seems to me that the list returned by the host_filesearch() is not going to change from call to call. Could we modify PATH only when the first thread enters this block and restore it when the last one leaves? |
|
Hm I think Regardless though this was something I didn't really want to invest much thought in, it doesn't really matter whether this is globally synchronized or has some logic like this, this is literally only exercised by |
|
@bors: r+ |
|
📌 Commit 1564e92 has been approved by |
rustc: Try to contain prepends to PATH This commit attempts to bring our prepends to PATH on Windows when loading plugins because we've been seeing quite a few issues with failing to spawn a process on Windows, the leading theory of which is that PATH is too large as a result of this. Currently this is mostly a stab in the dark as it's not confirmed to actually fix the problem, but it's probably not a bad change to have anyway! cc #33844 Closes #17360
This commit attempts to bring our prepends to PATH on Windows when loading
plugins because we've been seeing quite a few issues with failing to spawn a
process on Windows, the leading theory of which is that PATH is too large as a
result of this. Currently this is mostly a stab in the dark as it's not
confirmed to actually fix the problem, but it's probably not a bad change to
have anyway!
cc #33844
Closes #17360