Re: Potential problem in git-add may corrupt the index file

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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);

         fp = fopen(name, "w");
         if (!fp)
                 return error("cannot open %s", name);
// We may add
// fsync_or_die(fileno(fp), ...);
// here
         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, "+");

      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
      rename(path1, path0);

On Mon, Apr 23, 2012 at 9:51 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Gang Hu <ganghu@xxxxxxxxxxxxxxx> 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.

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at

[Newbies FAQ]     [Linux Kernel Development]     [Free Online Dating]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Free Online Dating]     [Photo]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]     [Linux Resources]

Add to Google