Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Nov 3, 2020 •
edited
Loading

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.

@CDirkx
Use is_verbatim_sep and is_sep_byte in parse_prefix
0ae630c 
For parsing `Prefix::DeviceNS` use `is_sep_byte` correctly, instead of `is_verbatim_sep`.
Also updates relevant testcase.
@rust-highfive
Copy link
Contributor

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 CDirkx mentioned this pull request Nov 7, 2020
Merged
@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
@Dylan-DPC
Rollup merge of rust-lang#78833 - CDirkx:parse_prefix, r=dtolnay
02e468e 
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
@Dylan-DPC
Rollup merge of rust-lang#78833 - CDirkx:parse_prefix, r=dtolnay
c42967b 
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
@Dylan-DPC
Rollup merge of rust-lang#78833 - CDirkx:parse_prefix, r=dtolnay
75c28fc 
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
@Dylan-DPC
Rollup merge of rust-lang#78833 - CDirkx:parse_prefix, r=dtolnay
12156e7 
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
@bors
Auto merge of rust-lang#78833 - CDirkx:parse_prefix, r=dtolnay
c00a464 
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
Reviewers
No reviews
Assignees

@kennytm kennytm

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@CDirkx @rust-highfive @kennytm @jyn514