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

Use is_sep_byte, instead of is_verbatim_sep when parsing Prefix::DeviceNS #78692

Closed
wants to merge 1 commit into from

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Nov 3, 2020

Currently on Windows, when parsing the prefix of a path in the device namespace, e.g. \\.\C:\foo.txt, only \ is recognized as a separator. This means that for the path \\.\C:/foo.txt, the entire path is parsed as the prefix, instead of only \\.\C:. This is also inconsistent because / is recognized as a separator in the rest of the path, e.g. \\.\C:\foo/bar.txt, just not in the prefix.

The documentation mentions no such restriction on the prefix, and prefixes delimited by / are handled fine on Windows:

use std::fs::File;
use std::io::prelude::*;

fn main() -> std::io::Result<()> {
    let mut file = File::create(r"\\.\C:/foo.txt")?;
    write!(file, "12345")?;
    drop(file);

    let mut file = File::open(r"C:\foo.txt")?;
    let mut contents = String::new();
    file.read_to_string(&mut contents)?;

    assert_eq!("12345", contents);

    Ok(())
}

Therefore it is my belief that the current behavior is a bug, which this PR addresses.

For parsing `Prefix::DeviceNS` use `is_sep_byte` correctly, instead of `is_verbatim_sep`.
Also updates relevant testcase.
@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 3, 2020
@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 3, 2020
@CDirkx
Copy link
Contributor Author

CDirkx commented Nov 7, 2020

I'm closing this PR in favor of #78833

@CDirkx CDirkx closed this Nov 7, 2020
@CDirkx CDirkx deleted the win-path branch November 8, 2020 15:24
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 14, 2020
Refactor and fix `parse_prefix` on Windows

This PR is an extension of rust-lang#78692 as well as a general refactor of `parse_prefix`:

**Fixes**:
There are two errors in the current implementation of `parse_prefix`:

Firstly, in the current implementation only `\` is recognized as a separator character in device namespace prefixes. This behavior is only correct for verbatim paths; `"\\.\C:/foo"` should be parsed as `"C:"` instead of `"C:/foo"`.

Secondly, the current implementation only handles single separator characters. In non-verbatim paths a series of separator characters should be recognized as a single boundary, e.g. the UNC path `"\\localhost\\\\\\C$\foo"` should be parsed as `"\\localhost\\\\\\C$"` and then `UNC(server: "localhost", share: "C$")`, but currently it is not parsed at all, because it starts being parsed as `\\localhost\` and then has an invalid empty share location.

Paths like `"\\.\C:/foo"` and `"\\localhost\\\\\\C$\foo"` are valid on Windows, they are equivalent to just `"C:\foo"`.

**Refactoring**:
All uses of `&[u8]` within `parse_prefix` are extracted to helper functions and`&OsStr` is used instead. This reduces the number of places unsafe is used:
- `get_first_two_components` is adapted to the more general `parse_next_component` and used in more places
- code for parsing drive prefixes is extracted to `parse_drive`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 15, 2020
Refactor and fix `parse_prefix` on Windows

This PR is an extension of rust-lang#78692 as well as a general refactor of `parse_prefix`:

**Fixes**:
There are two errors in the current implementation of `parse_prefix`:

Firstly, in the current implementation only `\` is recognized as a separator character in device namespace prefixes. This behavior is only correct for verbatim paths; `"\\.\C:/foo"` should be parsed as `"C:"` instead of `"C:/foo"`.

Secondly, the current implementation only handles single separator characters. In non-verbatim paths a series of separator characters should be recognized as a single boundary, e.g. the UNC path `"\\localhost\\\\\\C$\foo"` should be parsed as `"\\localhost\\\\\\C$"` and then `UNC(server: "localhost", share: "C$")`, but currently it is not parsed at all, because it starts being parsed as `\\localhost\` and then has an invalid empty share location.

Paths like `"\\.\C:/foo"` and `"\\localhost\\\\\\C$\foo"` are valid on Windows, they are equivalent to just `"C:\foo"`.

**Refactoring**:
All uses of `&[u8]` within `parse_prefix` are extracted to helper functions and`&OsStr` is used instead. This reduces the number of places unsafe is used:
- `get_first_two_components` is adapted to the more general `parse_next_component` and used in more places
- code for parsing drive prefixes is extracted to `parse_drive`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 15, 2020
Refactor and fix `parse_prefix` on Windows

This PR is an extension of rust-lang#78692 as well as a general refactor of `parse_prefix`:

**Fixes**:
There are two errors in the current implementation of `parse_prefix`:

Firstly, in the current implementation only `\` is recognized as a separator character in device namespace prefixes. This behavior is only correct for verbatim paths; `"\\.\C:/foo"` should be parsed as `"C:"` instead of `"C:/foo"`.

Secondly, the current implementation only handles single separator characters. In non-verbatim paths a series of separator characters should be recognized as a single boundary, e.g. the UNC path `"\\localhost\\\\\\C$\foo"` should be parsed as `"\\localhost\\\\\\C$"` and then `UNC(server: "localhost", share: "C$")`, but currently it is not parsed at all, because it starts being parsed as `\\localhost\` and then has an invalid empty share location.

Paths like `"\\.\C:/foo"` and `"\\localhost\\\\\\C$\foo"` are valid on Windows, they are equivalent to just `"C:\foo"`.

**Refactoring**:
All uses of `&[u8]` within `parse_prefix` are extracted to helper functions and`&OsStr` is used instead. This reduces the number of places unsafe is used:
- `get_first_two_components` is adapted to the more general `parse_next_component` and used in more places
- code for parsing drive prefixes is extracted to `parse_drive`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 15, 2020
Refactor and fix `parse_prefix` on Windows

This PR is an extension of rust-lang#78692 as well as a general refactor of `parse_prefix`:

**Fixes**:
There are two errors in the current implementation of `parse_prefix`:

Firstly, in the current implementation only `\` is recognized as a separator character in device namespace prefixes. This behavior is only correct for verbatim paths; `"\\.\C:/foo"` should be parsed as `"C:"` instead of `"C:/foo"`.

Secondly, the current implementation only handles single separator characters. In non-verbatim paths a series of separator characters should be recognized as a single boundary, e.g. the UNC path `"\\localhost\\\\\\C$\foo"` should be parsed as `"\\localhost\\\\\\C$"` and then `UNC(server: "localhost", share: "C$")`, but currently it is not parsed at all, because it starts being parsed as `\\localhost\` and then has an invalid empty share location.

Paths like `"\\.\C:/foo"` and `"\\localhost\\\\\\C$\foo"` are valid on Windows, they are equivalent to just `"C:\foo"`.

**Refactoring**:
All uses of `&[u8]` within `parse_prefix` are extracted to helper functions and`&OsStr` is used instead. This reduces the number of places unsafe is used:
- `get_first_two_components` is adapted to the more general `parse_next_component` and used in more places
- code for parsing drive prefixes is extracted to `parse_drive`
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 16, 2020
Refactor and fix `parse_prefix` on Windows

This PR is an extension of rust-lang#78692 as well as a general refactor of `parse_prefix`:

**Fixes**:
There are two errors in the current implementation of `parse_prefix`:

Firstly, in the current implementation only `\` is recognized as a separator character in device namespace prefixes. This behavior is only correct for verbatim paths; `"\\.\C:/foo"` should be parsed as `"C:"` instead of `"C:/foo"`.

Secondly, the current implementation only handles single separator characters. In non-verbatim paths a series of separator characters should be recognized as a single boundary, e.g. the UNC path `"\\localhost\\\\\\C$\foo"` should be parsed as `"\\localhost\\\\\\C$"` and then `UNC(server: "localhost", share: "C$")`, but currently it is not parsed at all, because it starts being parsed as `\\localhost\` and then has an invalid empty share location.

Paths like `"\\.\C:/foo"` and `"\\localhost\\\\\\C$\foo"` are valid on Windows, they are equivalent to just `"C:\foo"`.

**Refactoring**:
All uses of `&[u8]` within `parse_prefix` are extracted to helper functions and`&OsStr` is used instead. This reduces the number of places unsafe is used:
- `get_first_two_components` is adapted to the more general `parse_next_component` and used in more places
- code for parsing drive prefixes is extracted to `parse_drive`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants