[TempFile] Revert to using mkstemp on unix
#181
Conversation
roypat
requested review from
sameo,
andreeaflorescu and
jiangliu
as code owners
roypat
force-pushed
the
collision-free-tempfile
branch
3 times, most recently
from
194191d to
65de6df
Compare
src/tempfile.rs
Outdated
| let mut os_fname = prefix.as_ref().to_os_string(); | ||
| os_fname.push("XXXXXX"); | ||
|
|
||
| let raw_fname = CString::new(os_fname.into_vec()).unwrap().into_raw(); |
There was a problem hiding this comment.
Why is this unwrap safe? We should add a comment here to say that OsStr on unix is guaranteed to not have any non-zero bytes, and thus converting this to CString is safe, thus we can use unwrap.
There was a problem hiding this comment.
Okay I just checked the documentation and this seems to not be true (There is From<String> for OsString which does not allocate, and Strings are allowed to have nuls). Sooo I guess just using the old implementation does not work :(
Diving very deep into rust's stdlib, I came across how they handle this case, which seems to be std::io::ErrorKind::InvalidInput. After looking into more implementation details it seems that this would've previously been mapped to Err(Error(0)), so I'll mimic that behaviour
There was a problem hiding this comment.
Hmmm, this is rather unexpected. Nice dive deep!
Did you also check the implementation of From? It allows null bytes? 😱
There was a problem hiding this comment.
Yep, it eventually proxies through to this. I was quite surprised too, but I got suspicious after I couldn't find any docs telling me that "no nuls" was an invariant of OsString :(
|
@andreeaflorescu Do you want to me address your comments by amending the commit that |
I missed the commits history actually. Why do you want to handle this as a revert? It looks like we want to keep some of the functionality from the initial commit (i.e. the part where it works on Windows), so a revert might not be the best path forward in this case. I am opened to being changed my mind 😆 but for me right now it does not make sense to have this as a revert. |
In my head I treated it as a "revert the unix behavior change introduced by commit xxxxxx", but am happy to just treat it as a normal commit that |
roypat
force-pushed
the
collision-free-tempfile
branch
3 times, most recently
from
8230e74 to
04b06b7
Compare
roypat
force-pushed
the
collision-free-tempfile
branch
5 times, most recently
from
1e62d0e to
df11360
Compare
This partially reverts commit 2e77de9, as in that on unix we return to the behavior implemented before that commit was made. By using mkstemp, we prevent potential filename collisions from occuring (something firecracker occasionally observed when running heavily parallelized testing workloads). Code for the unix implementation is taken from the git history, with minor adjustments to meet the now-higher linting bar. On windows, we stick to the implementation of naming tempfiles after 6 random alphanumeric characters. Signed-off-by: Patrick Roy <[email protected]>
roypat
force-pushed
the
collision-free-tempfile
branch
from
df11360 to
434c8d3
Compare
Summary of the PR
The name generation in
TempFileis done by appending a randomly generated string of 6 alphanumeric characters to a prefix-path. This PR changes that implementation back ot usingmkstempon unix systems, to prevent name collisions. This additionally brings it inline with theTempDirimplementation in theunix::tempdirmodule.It seems that on windows something similar is possible via
GetTempFileName, but I am too unfamiliar with the win32 api (and also have no windows machine available for testing) to be confident in implementing that change.Reason
We encountered spurious failures in tests due to multiple parallel running tests generating the same random file name.
Open Questions
TempDir::new_with_prefixuses.truncate(true), which silently overwrites existing temporary files in the case of a name collision. Should this be changed?Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s), and the commitmessage has max 60 characters for the summary and max 75 characters for each
description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafecode is properly documented.