Re: [PATCH v2] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs
On Wed, Oct 30, 2013 at 05:19:27AM -0400, Jeff King wrote: > > To achieve this, I guess we have to follow the same procedure we do for > > loose object creation: > > > > 1. Create a temporary directory with a unique name (mkdtemp?) > > > > 2. Adjust permissions > > > > 3. Rename into place > > > > Can this be done sufficiently atomically across all platforms? > > Yeah, I think that is the only way to do it. I do not know offhand of > any platforms that have problems with atomic directory renames, though I > would not be surprised if some network filesystems don't handle it well. Actually, thinking on this more, we do not want "rename into place" semantics, as that usually implies overwrite. We would prefer a hard-link solution, where only one linker "wins", and the other gets EEXIST. But you cannot hard-link directories. POSIX does specify that rename() should return EEXIST rather than overwriting if the destination directory exists and is not empty. So in theory that would work, as we would then just be racing against the other process creating the actual object (and either we replace the directory with ours before they write the object, or they write the object first and we get EEXIST). But I don't know how well that is followed in the real world. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs
On Sun, Oct 27, 2013 at 12:35:43PM +0100, Johan Herland wrote: > I didn't see this in the latest "What's cooking", so here's a resend, with > an expanded commit message to reflect our discussion. The patch itself is > unchanged. Thanks, your expanded description looks correct to me. > In order to fix the remaining race, I assume we have to ensure the dir > creation obeys the same rules as the object creation, i.e. that there are > only two possible states at any time: > > - The directory does not exist > > - The directory exists with the correct permissons > > To achieve this, I guess we have to follow the same procedure we do for > loose object creation: > > 1. Create a temporary directory with a unique name (mkdtemp?) > > 2. Adjust permissions > > 3. Rename into place > > Can this be done sufficiently atomically across all platforms? Yeah, I think that is the only way to do it. I do not know offhand of any platforms that have problems with atomic directory renames, though I would not be surprised if some network filesystems don't handle it well. I'd also be fine if you want to simply leave it at your patch for now and let somebody who cares more about the other race worry about it later. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
