-
Notifications
You must be signed in to change notification settings - Fork 98
gvfs-helper: fix race condition when creating loose object dirs #205
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
gvfs-helper: fix race condition when creating loose object dirs #205
Conversation
When two gvfs-helper processes are the first to create a loose object directory, the processes (A and B in the timeline below) could have the following race: 1. A sees that the directory does not exist. 2. B sees that the directory does not exist. 3. A creates the directory with success. 4. B fails to create the directory and fails. Instead of having B fail here, just check for the directory's existence before reporting an error. That solves the race and allows tests to pass. Signed-off-by: Derrick Stolee <[email protected]>
@@ -1049,7 +1049,9 @@ static void create_tempfile_for_loose( | |||
strbuf_complete(&buf_path, '/'); | |||
strbuf_add(&buf_path, hex, 2); | |||
|
|||
if (!file_exists(buf_path.buf) && mkdir(buf_path.buf, 0777) == -1) { | |||
if (!file_exists(buf_path.buf) && |
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.
A bit unrelated, do you know if there is a function dir_exists
that checks if the path exists as a directory (or should we add one to use here)?
I couldn't find one in dir.c
, but I imagine all it would need is:
int dir_exists(const char *f)
{
struct stat sb;
if (lstat(f, &sb) != 0)
return false;
return S_ISDIR(sb.st_mode);
}
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.
The following exists in builtin/clone.c
:
static int dir_exists(const char *path)
{
struct stat sb;
return !stat(path, &sb);
}
It doesn't check the S_ISDIR()
.
I do see your interest in checking the type to make sure there isn't a file in the way. I'm not sure if there is enough value.
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'm not sure if there is enough value.
Yeah I was on the fence as well, we've not seen this be a problem thus far.
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.
Good catch. Thanks!
I'll fix it Monday. I think I can just try the mkdir() and catch the EEXIST error code.
I avoided that because of the platform nature of the error codes.
(or just try the lstat() again inside the error case.)
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.
Please note that there is not only a dir_exists()
, but also an is_directory()
function (the latter of which is public). There is even a ticket to reconcile them: gitgitgadget#230
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.
Approved with suggestion
…gvfs-helper Resolves #156. See microsoft/git#205 for the Git code change. One race condition still existed: who creates the loose object _directory_ first? The simple change is to check if the directory exists after the mkdir fails.
@derrickstolee Thanks for taking care of this!! |
When two gvfs-helper processes are the first to create a loose object
directory, the processes (A and B in the timeline below) could have
the following race:
Instead of having B fail here, just check for the directory's
existence before reporting an error. That solves the race and
allows tests to pass.