-
-
Notifications
You must be signed in to change notification settings - Fork 636
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 exclusive spawn mechanism for relative paths and working directories #14812
Fix exclusive spawn mechanism for relative paths and working directories #14812
Conversation
…m the working_directory. [ci skip-build-wheels]
cfe7469
to
4c8b26e
Compare
let mut executable_path = PathBuf::from(&req.argv[0]); | ||
if executable_path.is_relative() { | ||
if let Some(working_directory) = &req.working_directory { | ||
executable_path = working_directory.as_ref().join(executable_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be using RelativePath so that ../ are followed and collapsed and there is still verification the root has not been escaped. I can't see the harm if it is escaped, but the effort to avoid the lock is nominally being made, so go all the way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just saw this, sorry. I agree, but I also don't see the harm in letting it escape. Will let it slide for now to save a tree.
…ies. (pantsbuild#14812) The `exclusive_spawn` facility to avoid/retry for `ExecutableFileBusy` / "Text file busy" is triggered by having materialized `arg[0]` of a process into the process sandbox. But in the presence of a `Process::working_directory` and a relative path as `arg[0]`, the facility was not being triggered (since validation of `arg[0]` as a `RelativePath` would fail due to it escaping its root). This relates to pantsbuild#13424 (which would remove the need for the `exclusive_spawn` facility), but does not fix it: only ensure that we handle an existing known case. [ci skip-build-wheels]
…ies. (cherrypick of #14812) (#14816) The `exclusive_spawn` facility to avoid/retry for `ExecutableFileBusy` / "Text file busy" is triggered by having materialized `arg[0]` of a process into the process sandbox. But in the presence of a `Process::working_directory` and a relative path as `arg[0]`, the facility was not being triggered (since validation of `arg[0]` as a `RelativePath` would fail due to it escaping its root). This relates to #13424 (which would remove the need for the `exclusive_spawn` facility), but does not fix it: only ensure that we handle an existing known case. [ci skip-build-wheels]
The
exclusive_spawn
facility to avoid/retry forExecutableFileBusy
/ "Text file busy" is triggered by having materializedarg[0]
of a process into the process sandbox. But in the presence of aProcess::working_directory
and a relative path asarg[0]
, the facility was not being triggered (since validation ofarg[0]
as aRelativePath
would fail due to it escaping its root).This relates to #13424 (which would remove the need for the
exclusive_spawn
facility), but does not fix it: only ensure that we handle an existing known case.