-
Notifications
You must be signed in to change notification settings - Fork 135
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
Change directory to $*TMPDIR #732
base: master
Are you sure you want to change the base?
Conversation
e806121
to
202dc89
Compare
202dc89
to
9390d06
Compare
S32-io/IO-Socket-INET-UNIX.t
Outdated
my IO::Socket::INET:_ $server; | ||
my IO::Socket::INET:_ $client; | ||
my IO::Socket::INET:_ $accepted; | ||
my Str:D $host = $*TMPDIR.add("test-$*PID.sock").Str; | ||
my Str:D $host = "./test-$*PID.sock"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest a bit less generic name here. However little the probability for an entry with the same name to exists in the temp dir, I'd rather make it even smaller. To my view, the 100% reliable solution (unless race condition is an option) is to add just one line:
$host = "./test-$*PID-" ~ ++$ ~ ".sock" while -e $host;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, there is a risk of race conditions when checking in that way, unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I meant when added the "(unless ...)" part. But at least this problem is way less probable than with a static rather predictable name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured existing I/O tests would already have dealt with this sort of problem. S16-filehandles/io.t
opens files in the current directory and uses a less predictable filename:
sub nonce () { return ".{$*PID}." ~ (1..1000).pick() }
my $filename = 'tempfile_filehandles_io' ~ nonce();
But will silently continue with tests and remove the files if they already exist. I'm a bit wary of reusing this code. Do we always have permissions to write there?
b94756d
to
9390d06
Compare
If a file at the path already exists, tests will fail. We can't check for the existence of the file beforehand (racy!), so make it a little more difficult for that to happen in the same way S16-filehandles/io.t does for its test files.
My opinion: put temporary files into TMPDIR. That's what it's for. roast has
traditionally littered directories and I'd like to get rid of that. On the
contrary, it should set a good example.
As to safely creating a temp file, the best option is to rely on POSIX'
mkstemp() or mkstemps() functions. These combine secure file name generation
with atomic check and open.
The only safe alternative is to set the O_CREAT and O_EXCL flags, so that if
the file already exists, the openat() call errors with EEXIST and then loop
until you find an untaken name. The File::Temp module uses this approach.
Just for completeness as I don't think it would work here: on Linux even
better than mkstemp is openat() with the O_TMPFILE flag which creates an
unnamed temporary file, so there can never be any name clash and no security
issues due to predictable temp file names. Of course that doesn't work if you
need the file to be available to other processes. Unless those processes are
fork()ed or you pass the file descriptor via a pipe.
FWIW the whole discussion shows why I wished that we included secure temporary
file creation in Raku itself. Temporary files are a very common need and
creating them is something most people get wrong as the wronger a way is, the
easier it seems to be.
|
My bad, I haven't done it in first place, but a search in packages/Test-Helpers came up with |
This deduplicates some temporary file logic. Basenames generated by this routine are rather long (around 70 characters), but being in $*TMPDIR, we can use a relative path to give us more wiggle room.
4aeec0b
to
247c2af
Compare
S32-io/IO-Socket-INET-UNIX.t
Outdated
my IO::Socket::INET:_ $server; | ||
my IO::Socket::INET:_ $client; | ||
my IO::Socket::INET:_ $accepted; | ||
my Str:D $host = $*TMPDIR.add("test-$*PID.sock").Str; | ||
my Str:D $host = "./test-$*PID.sock"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I meant when added the "(unless ...)" part. But at least this problem is way less probable than with a static rather predictable name.
The TODO is now passing, and the new exception thrown is a better one.
Track changes caused by NQP PR #732
The TODO is now passing, and the new exception thrown is a better one.
The TODO is now passing, and the new exception thrown is a better one.
I tried out the patch and I'm afraid it doesn't really work as intended. For me (on FreeBSD 12.4) the socket files are created in the directory where I start I'm not totally sure about the why, but from what I've seen the C code that actually creates the socket file (IIUC it's the But maybe we should just use an absolute path instead of a relative one. (There's still the potential problem with |
This deduplicates some temporary file logic (and probably generates "better" file names). The main part of the patch was written by @Kaiepi -- see Raku#732. The only difference in this commit is the usage of an absolute path, instead of changing to $*TMPDIR and using a relative path. That's necessary because the C code that actually creates the socket file doesn't get its current working directory changed.
Alternative PR: #829 |
Fixes #718