This is an archive of the discontinued LLVM Phabricator instance.

[Support] fix TempFile infinite loop and permission denied errors
ClosedPublic

Authored by inglorion on Jul 31 2018, 7:23 PM.

Details

Summary

On Windows, TempFile::create() was prone to failing with permission
denied errors when a process created many tempfiles without providing
a model large enough to accommodate them. There was also a problem
with createUniqueEntity getting into an infinite loop when all names
permitted by the model are in use. This change fixes both of these
problems and adds a unit test for them.

Diff Detail

Repository
rL LLVM

Event Timeline

inglorion created this revision.Jul 31 2018, 7:23 PM

How could 16 characters not be enough? That’s 35^16 unique file names?

@zturner, the models we use here are 6 characters. Chosen from a set of 16 characters gives 16M possibilities. Randomly generating them gives a 50%+ probability of a collision after 5000 names or so. I have another patch up (D50127) to use a 32-character set instead, which means the same 6 character model gives about a billion possible names and you need to generate about 40,000 of them before you have a 50% probability of a collision.

This diff fixes a problem where, on Windows, TempFile::create() would immediately fail when the same name is tried again before the first file with that name is gone. That makes collisions less of an issue, because we can actually use more of the available names before we give up. It also fixes a problem where trying to create a tempfile when all available names are taken would lead to an infinite loop.

Is this good to go?

zturner accepted this revision.Aug 2 2018, 10:36 AM
This revision is now accepted and ready to land.Aug 2 2018, 10:36 AM
pcc requested changes to this revision.Aug 2 2018, 10:38 AM

No, I have comments.

This revision now requires changes to proceed.Aug 2 2018, 10:38 AM
This revision was not accepted when it landed; it landed in state Needs Revision.Aug 2 2018, 10:42 AM
This revision was automatically updated to reflect the committed changes.
pcc added inline comments.Aug 2 2018, 10:57 AM
llvm/lib/Support/Path.cpp
194 ↗(On Diff #158454)

This is unlikely to be the problem given the number of % characters typically in a model. It's more likely to be the case that we're getting permission denied because we don't actually have permission. That's what I'd say in the comment here.

Sorry, Peter. Saw your reply after I had already pushed the change. Would:

// Limit the number of attempts we make, so that we don't infinite loop. E.g.
// "permission denied" could be for a specific file (so we retry with a
// different name) or for the whole directory (retry would always fail).
// Checking which is racy, so we try a number of times, then give up.

be a better comment? If so, I'll land that as a follow-up.

pcc added a comment.Aug 2 2018, 11:22 AM

Looks great, thanks.

Comment changed in rL338755.

The test introduced here seems to be invalid.
It assumes that 16*128 attempts to choose a random hex number will cover all 16 available numbers.
Obviously, the chance for this test to fail is rather low but it is not 0.

(and yes, today it failed in my test run)

@fedor.sergeev: You're right, thanks for pointing that out. Sent D56336 to fix that.