Error when paths contain NUL characters #29913
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
I don't have a Windows machine right now, so this might not compile. |
| pub fn to_utf16(s: &Path) -> Vec<u16> { | ||
| s.as_os_str().encode_wide().chain(Some(0)).collect() | ||
| pub fn to_u16s(s: &Path) -> io::Result<Vec<u16>> { | ||
| if s.as_os_str().to_inner().to_inner().iter().any(|&b| b == 0) { |
There was a problem hiding this comment.
Could this check the u16 encoded version instead of the wtf-8 encoded version? The inner encoding may actually use a 0 when the u16 later becomes nonzero (I'm not sure).
There was a problem hiding this comment.
A feature of the old and new UTF-8 (the old UTF-8 corresponds to our WTF-8) is that all ASCII codepoints and only the ASCII codepoints are represented by the bytes 0 to 127. I can still change that if you want, though.
There was a problem hiding this comment.
While that may be true, yeah, it's more robust to check what we're actually returning rather than whatever encoding OsString happens to be in.
|
Can you add a test for this as well? Come to think of it I'm not sure we have a test for Unix either, so it may be good to have a test for that as well. |
|
Could you give me a hint as to where to place the test? In the same file? |
|
Ah nah I was thinking of a run-pass test which just makes a path with a null in it and make sure an error comes out and it doesn't succeed. |
|
Updated according to the comment, and added a test that runs on all OSs. |
| @@ -377,8 +377,13 @@ impl fmt::Debug for File { | |||
| } | |||
| } | |||
|
|
|||
| pub fn to_utf16(s: &Path) -> Vec<u16> { | |||
| s.as_os_str().encode_wide().chain(Some(0)).collect() | |||
| pub fn to_u16s(s: &Path) -> io::Result<Vec<u16>> { | |||
There was a problem hiding this comment.
Ah actually I was digging around the other day and ran across another to_utf16 function, could you merge this with that and update the signature there as well?
tbu-
force-pushed
the
pr_windows_path_error_on_nul
branch
from
81a91dd to
90ab4b5
Compare
| v.push(0); | ||
| v | ||
| pub fn to_u16s<S: AsRef<OsStr>>(s: &S) -> io::Result<Vec<u16>> { | ||
| fn inner(s: &OsStr) -> io::Result<Vec<u16>> { |
There was a problem hiding this comment.
In general this pattern isn't necessary for internal functions like this, I don't think there's much savings in terms of compile times for the standard library for example (and this definitely won't affect downstream code)
There was a problem hiding this comment.
I thought this pattern was used to reduce code size, so the outer, essentially no-op function can be inlined, and there are not many (in this case 2) of the monomorphized functions lying around.
There was a problem hiding this comment.
Ah yeah this is used for that but in this case it's so small and internal it likely doesn't matter anyway
|
Thanks! Can you also squash the commits? |
On Windows: Previously these paths were silently truncated at these NUL characters, now they fail with `ErrorKind::InvalidInput`.
tbu-
force-pushed
the
pr_windows_path_error_on_nul
branch
from
90ab4b5 to
bec42cf
Compare
|
Squashed into meaningful commits. Does "can you squash the commits?" generally mean: Squash into one commit or into more meaningful commits? |
|
@bors: r+ bec42cf7b40ac7abe0ff578ae76e4bea18072698 Ah yeah I'm personally always fine with just "squash away the temporary commits" moreso than "squash into one" |
|
⌛ Testing commit bec42cf with merge ca68a3b... |
|
💔 Test failed - auto-win-msvc-64-opt |
tbu-
force-pushed
the
pr_windows_path_error_on_nul
branch
from
bec42cf to
ee30f41
Compare
|
Fixed the compilation errors. |
|
@bors: r+ ee30f4127df0bb58d55d6f3af16abedc150875cd |
|
⌛ Testing commit ee30f41 with merge d7701a7... |
|
💔 Test failed - auto-win-msvc-32-opt |
tbu-
force-pushed
the
pr_windows_path_error_on_nul
branch
from
ee30f41 to
7386eae
Compare
|
Added a type annotation to help the type checker. |
|
@bors: r+ 7386eae |
|
⌛ Testing commit 7386eae with merge d901e1a... |
|
💔 Test failed - auto-win-msvc-64-opt |
tbu-
force-pushed
the
pr_windows_path_error_on_nul
branch
from
7386eae to
2cdd0e9
Compare
|
Removed unused import |
|
@bors: r+ 2cdd0e9 |
|
⌛ Testing commit 2cdd0e9 with merge b0d1f6d... |
|
💔 Test failed - auto-win-gnu-64-opt |
This check is necessary, because the underlying API only reads strings until the first NUL.
tbu-
force-pushed
the
pr_windows_path_error_on_nul
branch
from
2cdd0e9 to
71dccf8
Compare
|
Reintroduce |
On Windows: Previously these paths were silently truncated at these NUL characters, now they fail with `ErrorKind::InvalidInput`.
|
@alexcrichton Thanks for your patience! |
On Windows: Previously these paths were silently truncated at these NUL
characters, now they fail with
ErrorKind::InvalidInput.