Gang Hu [off-list ref] writes:
If a crash happens after the rename(), the data may have not been
completely flushed into the index file, so the user may face an empty
or corrupted index file.
Yeah, probably we should add fsync_or_die() at the end of write_index()
in read-cache.c, possibly protect it with fsync_object_files just like
we do in close_sha1_file() in sha1_file.c for loose object files.
I think the real fix would be to update the write_index() codepath to
use the csum-file API, though. Then sha1close() will give us the fsync
behaviour for free.
Hi Junio,
Thanks for the fast reply! We've also discovered other similar
programs in git, enclosed below so you may want to fix them in one
patch.
Here is one example in refs.c:
1710: lockpath = mkpath("%s.lock", git_HEAD);
fd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666);
....
written = write_in_full(fd, ref, len);
// We may add
// fsync_or_die(fd, ...);
// here
if (close(fd) != 0 || written != len) {
error("Unable to write to %s", lockpath);
goto error_unlink_return;
}
if (rename(lockpath, git_HEAD) < 0) {
...
So there is a problem here, and the head file may be corrupted.
In server-info.c, there is a similar problem:
216: namelen = sprintf(infofile, "%s/info/packs", get_object_directory());
strcpy(name, infofile);
strcpy(name + namelen, "+");
init_pack_info(infofile, force);
safe_create_leading_directories(name);
fp = fopen(name, "w");
if (!fp)
return error("cannot open %s", name);
write_pack_info_file(fp);
// We may add
// fsync_or_die(fileno(fp), ...);
// here
fclose(fp);
adjust_shared_perm(name);
rename(name, infofile);
Here, the packs file in the objects/info directory may be corrupted.
In the same file, the info/refs file may also be corrupted:
28: char *path0 = git_pathdup("info/refs");
int len = strlen(path0);
char *path1 = xmalloc(len + 2);
strcpy(path1, path0);
strcpy(path1 + len, "+");
safe_create_leading_directories(path0);
info_ref_fp = fopen(path1, "w");
if (!info_ref_fp)
return error("unable to update %s", path1);
for_each_ref(add_info_ref, NULL);
// We may add
// fsync_or_die(fileno(info_ref_fp), ...);
// here
fclose(info_ref_fp);
adjust_shared_perm(path1);
rename(path1, path0);
On Mon, Apr 23, 2012 at 9:51 PM, Junio C Hamano [off-list ref] wrote:Gang Hu [off-list ref] writes:
quoted
If a crash happens after the rename(), the data may have not been
completely flushed into the index file, so the user may face an empty
or corrupted index file.
Yeah, probably we should add fsync_or_die() at the end of write_index()
in read-cache.c, possibly protect it with fsync_object_files just like
we do in close_sha1_file() in sha1_file.c for loose object files.
I think the real fix would be to update the write_index() codepath to
use the csum-file API, though. Then sha1close() will give us the fsync
behaviour for free.
--
Cheers,
Gang