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

Process::new and dynamic_library::open_external should take &[u8]s, not Paths or ~strs #11650

Closed
erickt opened this issue Jan 18, 2014 · 6 comments · Fixed by #13954
Closed
Milestone

Comments

@erickt
Copy link
Contributor

erickt commented Jan 18, 2014

@kballard pointed out in IRC that the argument types for std::run::Process::new and std::unstable::dynamic_library::open_external are wrong for a couple reasons:

  • They don't quite work like proper paths. Process::new wraps the syscall execvp, which looks in the current directory for "./foo", but in the PATH search paths for "foo". open_external uses dlopen, does the same thing with the DL_LIBRARY_PATH or DYLD_LIBRARY_PATH environment variables. Since Path::new automatically normalizes the paths, it turns a local reference of "./foo" into "foo", which changes the semantics of the syscall.
  • In the case of Process::new, it uses ~str for the program and arguments, but on Unix paths have no encoding.
  • In the case of dynamic_library::open_external, on Macs you can embed special dyld macros as in "@executable_path/../foo", which will look for a library relative to the location of the executable.

Because of these issues, I feel that the only real option is to have those methods take &[u8], or a trait that can produce one, like std::path::BytesContainer.

@alexcrichton
Copy link
Member

Nominating.

@pnkfelix
Copy link
Member

Accepted for 1.0, P-backcompat-libs.

@lilyball
Copy link
Contributor

lilyball commented Feb 1, 2014

I'm considering this now a sub-task of #9639 (Clients of the new path need to be updated for non-utf8 paths). These could take &[u8], but we may instead want to make them take path::BytesContainer (which should probably be moved out of path at some point).

@aturon
Copy link
Member

aturon commented Apr 26, 2014

I'm planning to tackle this issue, but one thing wasn't clear to me from the original explanation: should both the program and arguments be changed to &[u8] (or path::BytesContainer) for Process::new, or only the program?

I suspect that both should be changed, since as far as I can tell Unix also doesn't specify an encoding for arguments.

@lilyball
Copy link
Contributor

Both should be changed.

@lilyball
Copy link
Contributor

Environment (in ProcessConfig) should also use [u8].

Of course, at this point, what we really want is to use Vec<u8> where appropriate. Process::new() should probably look like

fn new(prog: &[u8], args: &[Vec<u8>]) -> IoResult<Process>;

and ProcessConfig should look like

pub struct ProcessConfig<'a> {
    pub program: &'a [u8],
    pub args: &'a [Vec<u8>],
    pub env: Option<&'a [(Vec<u8>, Vec<u8>)]>,
    ...
}

aturon added a commit to aturon/rust that referenced this issue May 7, 2014
The existing APIs for spawning processes took strings for the command
and arguments, but the underlying system may not impose utf8 encoding,
so this is overly limiting.

The assumption we actually want to make is just that the command and
arguments are viewable as [u8] slices with no interior NULLs, i.e., as
CStrings. The ToCStr trait is a handy bound for types that meet this
requirement (such as &str and Path).

However, since the commands and arguments are often a mixture of
strings and paths, it would be inconvenient to take a slice with a
single T: ToCStr bound. So this patch revamps the process creation API
to instead use a builder-style interface, called `Command`, allowing
arguments to be added one at a time with differing ToCStr
implementations for each.

The initial cut of the builder API has some drawbacks that can be
addressed once issue rust-lang#13851 (libstd as a facade) is closed. These are
detailed as FIXMEs.

Closes rust-lang#11650.

[breaking-change]
aturon added a commit to aturon/rust that referenced this issue May 7, 2014
`std::unstable::dynamic_library::open_external` currently takes a
`Path`, but because `Paths` produce normalized strings, this can
change the semantics of lookups in a given environment. This patch
generalizes the function to take a `ToCStr`-bounded type, which
includes both `Path`s and `str`s.

Closes rust-lang#11650.
aturon added a commit that referenced this issue May 12, 2014
`std::unstable::dynamic_library::open_external` currently takes a
`Path`, but because `Paths` produce normalized strings, this can
change the semantics of lookups in a given environment. This patch
generalizes the function to take a `ToCStr`-bounded type, which
includes both `Path`s and `str`s.

Closes #11650.
aturon added a commit that referenced this issue May 13, 2014
The existing APIs for spawning processes took strings for the command
and arguments, but the underlying system may not impose utf8 encoding,
so this is overly limiting.

The assumption we actually want to make is just that the command and
arguments are viewable as [u8] slices with no interior NULLs, i.e., as
CStrings. The ToCStr trait is a handy bound for types that meet this
requirement (such as &str and Path).

However, since the commands and arguments are often a mixture of
strings and paths, it would be inconvenient to take a slice with a
single T: ToCStr bound. So this patch revamps the process creation API
to instead use a builder-style interface, called `Command`, allowing
arguments to be added one at a time with differing ToCStr
implementations for each.

The initial cut of the builder API has some drawbacks that can be
addressed once issue #13851 (libstd as a facade) is closed. These are
detailed as FIXMEs.

Closes #11650.

[breaking-change]
aturon added a commit that referenced this issue May 13, 2014
`std::unstable::dynamic_library::open_external` currently takes a
`Path`, but because `Paths` produce normalized strings, this can
change the semantics of lookups in a given environment. This patch
generalizes the function to take a `ToCStr`-bounded type, which
includes both `Path`s and `str`s.

Closes #11650.
aturon added a commit to aturon/rust that referenced this issue May 14, 2014
The existing APIs for spawning processes took strings for the command
and arguments, but the underlying system may not impose utf8 encoding,
so this is overly limiting.

The assumption we actually want to make is just that the command and
arguments are viewable as [u8] slices with no interior NULLs, i.e., as
CStrings. The ToCStr trait is a handy bound for types that meet this
requirement (such as &str and Path).

However, since the commands and arguments are often a mixture of
strings and paths, it would be inconvenient to take a slice with a
single T: ToCStr bound. So this patch revamps the process creation API
to instead use a builder-style interface, called `Command`, allowing
arguments to be added one at a time with differing ToCStr
implementations for each.

The initial cut of the builder API has some drawbacks that can be
addressed once issue rust-lang#13851 (libstd as a facade) is closed. These are
detailed as FIXMEs.

Closes rust-lang#11650.

[breaking-change]
aturon added a commit to aturon/rust that referenced this issue May 14, 2014
`std::unstable::dynamic_library::open_external` currently takes a
`Path`, but because `Paths` produce normalized strings, this can
change the semantics of lookups in a given environment. This patch
generalizes the function to take a `ToCStr`-bounded type, which
includes both `Path`s and `str`s.

Closes rust-lang#11650.
aturon added a commit that referenced this issue May 14, 2014
The existing APIs for spawning processes took strings for the command
and arguments, but the underlying system may not impose utf8 encoding,
so this is overly limiting.

The assumption we actually want to make is just that the command and
arguments are viewable as [u8] slices with no interior NULLs, i.e., as
CStrings. The ToCStr trait is a handy bound for types that meet this
requirement (such as &str and Path).

However, since the commands and arguments are often a mixture of
strings and paths, it would be inconvenient to take a slice with a
single T: ToCStr bound. So this patch revamps the process creation API
to instead use a builder-style interface, called `Command`, allowing
arguments to be added one at a time with differing ToCStr
implementations for each.

The initial cut of the builder API has some drawbacks that can be
addressed once issue #13851 (libstd as a facade) is closed. These are
detailed as FIXMEs.

Closes #11650.

[breaking-change]
aturon added a commit that referenced this issue May 14, 2014
`std::unstable::dynamic_library::open_external` currently takes a
`Path`, but because `Paths` produce normalized strings, this can
change the semantics of lookups in a given environment. This patch
generalizes the function to take a `ToCStr`-bounded type, which
includes both `Path`s and `str`s.

Closes #11650.
aturon added a commit that referenced this issue May 15, 2014
The existing APIs for spawning processes took strings for the command
and arguments, but the underlying system may not impose utf8 encoding,
so this is overly limiting.

The assumption we actually want to make is just that the command and
arguments are viewable as [u8] slices with no interior NULLs, i.e., as
CStrings. The ToCStr trait is a handy bound for types that meet this
requirement (such as &str and Path).

However, since the commands and arguments are often a mixture of
strings and paths, it would be inconvenient to take a slice with a
single T: ToCStr bound. So this patch revamps the process creation API
to instead use a builder-style interface, called `Command`, allowing
arguments to be added one at a time with differing ToCStr
implementations for each.

The initial cut of the builder API has some drawbacks that can be
addressed once issue #13851 (libstd as a facade) is closed. These are
detailed as FIXMEs.

Closes #11650.

[breaking-change]
aturon added a commit that referenced this issue May 15, 2014
`std::unstable::dynamic_library::open_external` currently takes a
`Path`, but because `Paths` produce normalized strings, this can
change the semantics of lookups in a given environment. This patch
generalizes the function to take a `ToCStr`-bounded type, which
includes both `Path`s and `str`s.

Closes #11650.
bors added a commit that referenced this issue May 15, 2014
## Process API

The existing APIs for spawning processes took strings for the command
and arguments, but the underlying system may not impose utf8 encoding,
so this is overly limiting.

The assumption we actually want to make is just that the command and
arguments are viewable as [u8] slices with no interior NULLs, i.e., as
CStrings. The ToCStr trait is a handy bound for types that meet this
requirement (such as &str and Path).

However, since the commands and arguments are often a mixture of
strings and paths, it would be inconvenient to take a slice with a
single T: ToCStr bound. So this patch revamps the process creation API
to instead use a builder-style interface, called `Command`, allowing
arguments to be added one at a time with differing ToCStr
implementations for each.

The initial cut of the builder API has some drawbacks that can be
addressed once issue #13851 (libstd as a facade) is closed. These are
detailed as FIXMEs.

## Dynamic library API

`std::unstable::dynamic_library::open_external` currently takes a
`Path`, but because `Paths` produce normalized strings, this can
change the semantics of lookups in a given environment. This patch
generalizes the function to take a `ToCStr`-bounded type, which
includes both `Path`s and `str`s.

## ToCStr API

Adds ToCStr impl for &Path and ~str. This is a stopgap until DST (#12938) lands.

Until DST lands, we cannot decompose &str into & and str, so we cannot
usefully take ToCStr arguments by reference (without forcing an
additional & around &str). So we are instead temporarily adding an
instance for &Path and ~str, so that we can take ToCStr as owned. When
DST lands, the &Path instance should be removed, the string instances
should be revisted, and arguments bound by ToCStr should be passed by
reference.

FIXMEs have been added accordingly. 

## Tickets closed

Closes #11650.
Closes #7928.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants