{"thread":{"id":"30314","subject":"Potential problem in git-add may corrupt the index file","startedAt":"2012-04-23T23:30:05Z","lastAt":"2012-04-29T22:39:23Z","messageCount":3,"participants":["Gang Hu","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"189919","messageId":"CAEJt7hYYJh5dT1Tk99Oze6Hg4QaOqX5ERi0faWjyifQ6dr-pWw@mail.gmail.com","threadId":"30314","inReplyTo":null,"subject":"Potential problem in git-add may corrupt the index file","fromName":"Gang Hu","fromEmail":"ganghu@cs.columbia.edu","sentAt":"2012-04-23T23:30:05Z","receivedAt":"2012-04-23T23:30:05Z","isPatch":false,"sender":{"key":"ganghu@cs.columbia.edu","avatar":null},"body":"Hi,\n\nWe are developing a tool to find bugs, and it flagged a potential\nproblem in git-add.\n\nWe are checking git 1.7.9.4, and this problem still exists in the\nlatest release, git 1.7.10.\nAt the end of cmd_add() in builtin/add.c, there is a call to\nwrite_cache(), followed by commit_locked_index().\n\n474:            if (write_cache(newfd, active_cache, active_nr) ||\n475:                commit_locked_index(&lock_file))\n\nIn write_cache(), the program wrote to the locked index file, and in\ncommit_locked_index(), it did an atomic rename\nto rename the locked index file to the real index file.\nHowever, the locked index file is not synced to the disk before\nbeing renamed to the real index file. This may\ncause data loss, which has been pointed out in this post:\n\nhttp://stackoverflow.com/questions/7433057/is-rename-without-fsync-safe\n\nIf a crash happens after the rename(), the data may have not been\ncompletely flushed into the index file, so the user may face an empty\nor corrupted index file.\nAlthough the user may just recreate the index file, he would need to\nadd the files to be committed again, which may be inconvenient and disturbing.\n\nTo fix this problem, we may just do a fsync() before renaming the\nfile. We can add this to close_lock_file() in lockfile.c:\n223: int close_lock_file(struct lock_file *lk)\n224: {\n225:        int fd = lk->fd;\n226:        lk->fd = -1;\n          // we may add\n          // fsync(fd);\n          // here\n227:        return close(fd);\n228:}\n\nIs this a real problem? Any clarification or confirmation is\nappreciated. Thank you.\n\n-- \nCheers,\nGang\n"},{"id":"189923","messageId":"xmqq4ns9j3rf.fsf@junio.mtv.corp.google.com","threadId":"30314","inReplyTo":"CAEJt7hYYJh5dT1Tk99Oze6Hg4QaOqX5ERi0faWjyifQ6dr-pWw@mail.gmail.com","subject":"Re: Potential problem in git-add may corrupt the index file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-24T01:51:48Z","receivedAt":"2012-04-24T01:51:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Gang Hu <ganghu@cs.columbia.edu> writes:\n\n> If a crash happens after the rename(), the data may have not been\n> completely flushed into the index file, so the user may face an empty\n> or corrupted index file.\n\nYeah, probably we should add fsync_or_die() at the end of write_index()\nin read-cache.c, possibly protect it with fsync_object_files just like\nwe do in close_sha1_file() in sha1_file.c for loose object files.\n\nI think the real fix would be to update the write_index() codepath to\nuse the csum-file API, though.  Then sha1close() will give us the fsync\nbehaviour for free.\n"},{"id":"190258","messageId":"CAEJt7hbKFKN8J5jjX9Z_Apnco+YiN0kYn=wrHTYjE9P9M=jjaw@mail.gmail.com","threadId":"30314","inReplyTo":"xmqq4ns9j3rf.fsf@junio.mtv.corp.google.com","subject":"Re: Potential problem in git-add may corrupt the index file","fromName":"Gang Hu","fromEmail":"ganghu@cs.columbia.edu","sentAt":"2012-04-29T22:39:23Z","receivedAt":"2012-04-29T22:39:23Z","isPatch":false,"sender":{"key":"ganghu@cs.columbia.edu","avatar":null},"body":"Hi Junio,\n\nThanks for the fast reply!  We've also discovered other similar\nprograms in git, enclosed below so you may want to fix them in one\npatch.\n\nHere is one example in refs.c:\n1710:  lockpath = mkpath(\"%s.lock\", git_HEAD);\n        fd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666);\n....\n      written = write_in_full(fd, ref, len);\n// We may add\n// fsync_or_die(fd, ...);\n// here\n      if (close(fd) != 0 || written != len) {\n              error(\"Unable to write to %s\", lockpath);\n              goto error_unlink_return;\n      }\n      if (rename(lockpath, git_HEAD) < 0) {\n...\n\nSo there is a problem here, and the head file may be corrupted.\n\nIn server-info.c, there is a similar problem:\n216:   namelen = sprintf(infofile, \"%s/info/packs\", get_object_directory());\n         strcpy(name, infofile);\n         strcpy(name + namelen, \"+\");\n\n         init_pack_info(infofile, force);\n\n         safe_create_leading_directories(name);\n         fp = fopen(name, \"w\");\n         if (!fp)\n                 return error(\"cannot open %s\", name);\n         write_pack_info_file(fp);\n// We may add\n// fsync_or_die(fileno(fp), ...);\n// here\n         fclose(fp);\n         adjust_shared_perm(name);\n         rename(name, infofile);\nHere, the packs file in the objects/info directory may be corrupted.\n\nIn the same file, the info/refs file may also be corrupted:\n28:  char *path0 = git_pathdup(\"info/refs\");\n      int len = strlen(path0);\n      char *path1 = xmalloc(len + 2);\n\n      strcpy(path1, path0);\n      strcpy(path1 + len, \"+\");\n\n      safe_create_leading_directories(path0);\n      info_ref_fp = fopen(path1, \"w\");\n      if (!info_ref_fp)\n              return error(\"unable to update %s\", path1);\n      for_each_ref(add_info_ref, NULL);\n// We may add\n// fsync_or_die(fileno(info_ref_fp), ...);\n// here\n      fclose(info_ref_fp);\n      adjust_shared_perm(path1);\n      rename(path1, path0);\n\nOn Mon, Apr 23, 2012 at 9:51 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Gang Hu <ganghu@cs.columbia.edu> writes:\n>\n>> If a crash happens after the rename(), the data may have not been\n>> completely flushed into the index file, so the user may face an empty\n>> or corrupted index file.\n>\n> Yeah, probably we should add fsync_or_die() at the end of write_index()\n> in read-cache.c, possibly protect it with fsync_object_files just like\n> we do in close_sha1_file() in sha1_file.c for loose object files.\n>\n> I think the real fix would be to update the write_index() codepath to\n> use the csum-file API, though.  Then sha1close() will give us the fsync\n> behaviour for free.\n\n\n\n-- \nCheers,\nGang\n"}]}