{"thread":{"id":"34215","subject":"[PATCH v2 0/3] Fix a race condition when reading loose refs","startedAt":"2013-06-19T06:36:25Z","lastAt":"2013-06-19T17:24:38Z","messageCount":6,"participants":["Michael Haggerty","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"221338","messageId":"1371623788-7227-1-git-send-email-mhagger@alum.mit.edu","threadId":"34215","inReplyTo":null,"subject":"[PATCH v2 0/3] Fix a race condition when reading loose refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-19T06:36:25Z","receivedAt":"2013-06-19T06:36:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is v2 of mh/loose-refs-race-with-pack-ref\n\nI took Peff's suggestion to use gotos rather than an infinite loop in\nthe last patch, which means that there is no need for the old patch\n03/04.\n\nMichael Haggerty (3):\n resolve_ref_unsafe(): extract function handle_missing_loose_ref()\n resolve_ref_unsafe(): handle the case of an SHA-1 within loop\n resolve_ref_unsafe(): close race condition reading loose refs\n\n refs.c | 106 ++++++++++++++++++++++++++++++++++++++++++++---------------------\n 1 file changed, 72 insertions(+), 34 deletions(-)\n\n-- \n1.8.3.1\n"},{"id":"221339","messageId":"1371623788-7227-2-git-send-email-mhagger@alum.mit.edu","threadId":"34215","inReplyTo":"1371623788-7227-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 1/3] resolve_ref_unsafe(): extract function handle_missing_loose_ref()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-19T06:36:26Z","receivedAt":"2013-06-19T06:36:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The nesting was getting a bit out of hand, and it's about to get\nworse.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 56 +++++++++++++++++++++++++++++++++++---------------------\n 1 file changed, 35 insertions(+), 21 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 42a7e17..ab1f99e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1197,6 +1197,37 @@ static struct ref_entry *get_packed_ref(const char *refname)\n \treturn find_ref(get_packed_refs(&ref_cache), refname);\n }\n \n+/*\n+ * A loose ref file doesn't exist; check for a packed ref. The\n+ * options are forwarded from resolve_safe_unsafe().\n+ */\n+static const char *handle_missing_loose_ref(const char *refname,\n+\t\t\t\t\t unsigned char *sha1,\n+\t\t\t\t\t int reading,\n+\t\t\t\t\t int *flag)\n+{\n+\tstruct ref_entry *entry;\n+\n+\t/*\n+\t * The loose reference file does not exist; check for a packed\n+\t * reference.\n+\t */\n+\tentry = get_packed_ref(refname);\n+\tif (entry) {\n+\t\thashcpy(sha1, entry->u.value.sha1);\n+\t\tif (flag)\n+\t\t\t*flag |= REF_ISPACKED;\n+\t\treturn refname;\n+\t}\n+\t/* The reference is not a packed reference, either. */\n+\tif (reading) {\n+\t\treturn NULL;\n+\t} else {\n+\t\thashclr(sha1);\n+\t\treturn refname;\n+\t}\n+}\n+\n const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)\n {\n \tint depth = MAXDEPTH;\n@@ -1222,28 +1253,11 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\tgit_snpath(path, sizeof(path), \"%s\", refname);\n \n \t\tif (lstat(path, &st) < 0) {\n-\t\t\tstruct ref_entry *entry;\n-\n-\t\t\tif (errno != ENOENT)\n+\t\t\tif (errno == ENOENT)\n+\t\t\t\treturn handle_missing_loose_ref(refname, sha1,\n+\t\t\t\t\t\t\t\treading, flag);\n+\t\t\telse\n \t\t\t\treturn NULL;\n-\t\t\t/*\n-\t\t\t * The loose reference file does not exist;\n-\t\t\t * check for a packed reference.\n-\t\t\t */\n-\t\t\tentry = get_packed_ref(refname);\n-\t\t\tif (entry) {\n-\t\t\t\thashcpy(sha1, entry->u.value.sha1);\n-\t\t\t\tif (flag)\n-\t\t\t\t\t*flag |= REF_ISPACKED;\n-\t\t\t\treturn refname;\n-\t\t\t}\n-\t\t\t/* The reference is not a packed reference, either. */\n-\t\t\tif (reading) {\n-\t\t\t\treturn NULL;\n-\t\t\t} else {\n-\t\t\t\thashclr(sha1);\n-\t\t\t\treturn refname;\n-\t\t\t}\n \t\t}\n \n \t\t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n-- \n1.8.3.1\n"},{"id":"221340","messageId":"1371623788-7227-3-git-send-email-mhagger@alum.mit.edu","threadId":"34215","inReplyTo":"1371623788-7227-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 2/3] resolve_ref_unsafe(): handle the case of an SHA-1 within loop","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-19T06:36:27Z","receivedAt":"2013-06-19T06:36:27Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"There is only one \"break\" statement within the loop, which jumps to\nthe code after the loop that handles the case of a file that holds a\nSHA-1. So move that code from below the loop into the if statement\nwhere the break was previously located. This makes the logic flow\nmore local.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 22 +++++++++++++---------\n 1 file changed, 13 insertions(+), 9 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ab1f99e..867cf9f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1300,8 +1300,19 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t/*\n \t\t * Is it a symbolic ref?\n \t\t */\n-\t\tif (prefixcmp(buffer, \"ref:\"))\n-\t\t\tbreak;\n+\t\tif (prefixcmp(buffer, \"ref:\")) {\n+\t\t\t/*\n+\t\t\t * Please note that FETCH_HEAD has a second\n+\t\t\t * line containing other data.\n+\t\t\t */\n+\t\t\tif (get_sha1_hex(buffer, sha1) ||\n+\t\t\t (buffer[40] != '\\0' && !isspace(buffer[40]))) {\n+\t\t\t\tif (flag)\n+\t\t\t\t\t*flag |= REF_ISBROKEN;\n+\t\t\t\treturn NULL;\n+\t\t\t}\n+\t\t\treturn refname;\n+\t\t}\n \t\tif (flag)\n \t\t\t*flag |= REF_ISSYMREF;\n \t\tbuf = buffer + 4;\n@@ -1314,13 +1325,6 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t}\n \t\trefname = strcpy(refname_buffer, buf);\n \t}\n-\t/* Please note that FETCH_HEAD has a second line containing other data. */\n-\tif (get_sha1_hex(buffer, sha1) || (buffer[40] != '\\0' && !isspace(buffer[40]))) {\n-\t\tif (flag)\n-\t\t\t*flag |= REF_ISBROKEN;\n-\t\treturn NULL;\n-\t}\n-\treturn refname;\n }\n \n char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag)\n-- \n1.8.3.1\n"},{"id":"221341","messageId":"1371623788-7227-4-git-send-email-mhagger@alum.mit.edu","threadId":"34215","inReplyTo":"1371623788-7227-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 3/3] resolve_ref_unsafe(): close race condition reading loose refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-19T06:36:28Z","receivedAt":"2013-06-19T06:36:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"We read loose references in two steps. The code is roughly:\n\n lstat()\n if error ENOENT:\n loose ref is missing; look for corresponding packed ref\n else if S_ISLNK:\n readlink()\n if error:\n report failure\n else if S_ISDIR:\n report failure\n else\n open()\n if error:\n report failure\n read()\n\nThe problem is that the first filesystem call, to lstat(), is not\natomic with the second filesystem call, to readlink() or open().\nTherefore it is possible for another process to change the file\nbetween our two calls, for example:\n\n* If the other process deletes the file, our second call will fail\n with ENOENT, which we *should* interpret as \"loose ref is missing;\n look for corresponding packed ref\". This can arise if the other\n process is pack-refs; it might have just written a new packed-refs\n file containing the old contents of the reference then deleted the\n loose ref.\n\n* If the other process changes a symlink into a plain file, our call\n to readlink() will fail with EINVAL, which we *should* respond to by\n trying to open() and read() the file.\n\nThe old code treats the reference as missing in both of these cases,\nwhich is incorrect.\n\nSo instead, handle errors more selectively: if the result of\nreadline()/open() is a failure that is inconsistent with the result of\nthe previous lstat(), then something is fishy. In this case jump back\nand start over again with a fresh call to lstat().\n\nOne race is still possible and undetected: another process could\nchange the file from a regular file into a symlink between the call to\nlstat and the call to open(). The open() call would silently follow\nthe symlink and not know that something is wrong. This situation\ncould be detected in two ways:\n\n* On systems that support O_NOFOLLOW, pass that option to the open().\n\n* On other systems, call fstat() on the fd returned by open() and make\n sure that it agrees with the stat info from the original lstat().\n\nHowever, we don't use symlinks anymore, so this situation is unlikely.\nMoreover, it doesn't appear that treating a symlink as a regular file\nwould have grave consequences; after all, this is exactly how the code\nhandles non-relative symlinks. So this commit leaves that race\nunaddressed.\n\nNote that this solves only the part of the race within\nresolve_ref_unsafe. In the situation described above, we may still be\ndepending on a cached view of the packed-refs file; that race will be\ndealt with in a future patch.\n\nThis problem was reported and diagnosed by Jeff King ,\nand this solution is derived from his patch.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 28 ++++++++++++++++++++++++----\n 1 file changed, 24 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 867cf9f..70b2c82 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1252,6 +1252,16 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \n \t\tgit_snpath(path, sizeof(path), \"%s\", refname);\n \n+\t\t/*\n+\t\t * We might have to loop back here to avoid a race\n+\t\t * condition: first we lstat() the file, then we try\n+\t\t * to read it as a link or as a file. But if somebody\n+\t\t * changes the type of the file (file <-> directory\n+\t\t * <-> symlink) between the lstat() and reading, then\n+\t\t * we don't want to report that as an error but rather\n+\t\t * try again starting with the lstat().\n+\t\t */\n+\tstat_ref:\n \t\tif (lstat(path, &st) < 0) {\n \t\t\tif (errno == ENOENT)\n \t\t\t\treturn handle_missing_loose_ref(refname, sha1,\n@@ -1263,8 +1273,13 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n \t\tif (S_ISLNK(st.st_mode)) {\n \t\t\tlen = readlink(path, buffer, sizeof(buffer)-1);\n-\t\t\tif (len < 0)\n-\t\t\t\treturn NULL;\n+\t\t\tif (len < 0) {\n+\t\t\t\tif (errno == ENOENT || errno == EINVAL)\n+\t\t\t\t\t/* inconsistent with lstat; retry */\n+\t\t\t\t\tgoto stat_ref;\n+\t\t\t\telse\n+\t\t\t\t\treturn NULL;\n+\t\t\t}\n \t\t\tbuffer[len] = 0;\n \t\t\tif (!prefixcmp(buffer, \"refs/\") &&\n \t\t\t\t\t!check_refname_format(buffer, 0)) {\n@@ -1287,8 +1302,13 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t * a ref\n \t\t */\n \t\tfd = open(path, O_RDONLY);\n-\t\tif (fd < 0)\n-\t\t\treturn NULL;\n+\t\tif (fd < 0) {\n+\t\t\tif (errno == ENOENT)\n+\t\t\t\t/* inconsistent with lstat; retry */\n+\t\t\t\tgoto stat_ref;\n+\t\t\telse\n+\t\t\t\treturn NULL;\n+\t\t}\n \t\tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n \t\tclose(fd);\n \t\tif (len < 0)\n-- \n1.8.3.1\n"},{"id":"221381","messageId":"20130619160150.GA19139@sigill.intra.peff.net","threadId":"34215","inReplyTo":"1371623788-7227-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 0/3] Fix a race condition when reading loose refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-19T16:01:51Z","receivedAt":"2013-06-19T16:01:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 19, 2013 at 08:36:25AM +0200, Michael Haggerty wrote:\n\n> I took Peff's suggestion to use gotos rather than an infinite loop in\n> the last patch, which means that there is no need for the old patch\n> 03/04.\n\nThanks, this version looks good to me.\n\nI'm sure the Pascal programmers of the world collectively sighed in\ndisgust at a code review requesting a for loop turn into a goto, but I\nthink it is more readable than the first version. :)\n\n-Peff\n"},{"id":"221389","messageId":"7vd2ri10a1.fsf@alter.siamese.dyndns.org","threadId":"34215","inReplyTo":"20130619160150.GA19139@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/3] Fix a race condition when reading loose refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-19T17:24:38Z","receivedAt":"2013-06-19T17:24:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jun 19, 2013 at 08:36:25AM +0200, Michael Haggerty wrote:\n>\n>> I took Peff's suggestion to use gotos rather than an infinite loop in\n>> the last patch, which means that there is no need for the old patch\n>> 03/04.\n>\n> Thanks, this version looks good to me.\n>\n> I'm sure the Pascal programmers of the world collectively sighed in\n> disgust at a code review requesting a for loop turn into a goto, but I\n> think it is more readable than the first version. :)\n>\n> -Peff\n\nThanks, both. Essentially the first two are the same from the\nprevious round, and the third one gives us \"oops, unexpected\nstate---let's retry\" in a more straight-forward way.\n\nLooks very sensible; will queue.\n"}]}