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

tempname, mktemp: test uniqueness of names #33765

Closed
wants to merge 1 commit into from

Conversation

StefanKarpinski
Copy link
Member

No description provided.

@Keno
Copy link
Member

Keno commented Nov 5, 2019

If I'm reading the docs correctly, the problem is that we pass 0 here rather than a random number, which uses the current time to select the seed (so it conflicts if you do it too quickly) as well as not checking if the name is actually unique:

filename = _win_tempname(parent, UInt32(0))

Should probably be rand(UInt32) as in the next function.

@davidanthoff
Copy link
Contributor

I think the problem is not in mktemp, but in tempname, i.e. I think passing the 0 to the call of _win_tempname in mktemp is actually correct, and the approach in tempname with the random number generation is the problem. The tests also fail for the tempname case, so that seems to be the problem.

According to the docs, if you pass a 0 there, it will actually verify the uniqueness of the filename. The way it works in that case is that it will start with a number based on the current system time. It then checks whether a file with that name exists. If yes, it will try a new number. If no, it will create an empty file with that name and close it. So that algorithm should guarantee that it will never return the same name twice, because it will create a file on disc with any name that it returned, so at that point that filename is "blocked" from returning again.

The random approach in tempname generates the unique number in Julia, rather than delegating to Windows. It also creates random numbers, and tests whether a file with that name already exists. BUT, unlike the Windows API function, it doesn't create a file on disc if it returns you a given temp filename. So at no place is there a record kept of "this filename is already used now", so if you call tempname repeatedly (as in these tests) you can super easily get the same filename returned if you end up getting the same random number twice (especially given that it only samples in the domain 0..65k).

If the idea for tempname is that it should continue to not create anything on disc, then the easiest option seems to be to follow the suggestion in the MS docs and use some UUID for the temporary filename, and just not use the GetTempFileNameW function at all for that case.

@davidanthoff
Copy link
Contributor

Here is the equivalent .Net implementation, it seems to me that an approach like that would be the safest option for tempname and clearly the path that MS is recommending and using itself.

@vtjnash
Copy link
Member

vtjnash commented Nov 7, 2019

From that link (a few lines further down), it looks like .net uses GetTempFileName like we are

@davidanthoff
Copy link
Contributor

From that link (a few lines further down), it looks like .net uses GetTempFileName like we are

They only call GetTempFileName with uUnique=0, which is (sort of) fine, and what julia does in mktemp. I don't see any example of them doing anything like what is done in Julia's tempname, which is where the problem is.

@StefanKarpinski
Copy link
Member Author

Included in #33785

@StefanKarpinski StefanKarpinski deleted the sk/fix-win32-tempname branch January 28, 2020 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants