{"thread":{"id":"36385","subject":"[PATCH] Verify index file before we opportunistically update it","startedAt":"2014-04-09T22:06:36Z","lastAt":"2014-04-14T18:50:44Z","messageCount":31,"participants":["Yiannis Marangos","Junio C Hamano","Duy Nguyen","Johannes Sixt","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"238606","messageId":"1397081197-14803-1-git-send-email-yiannis.marangos@gmail.com","threadId":"36385","inReplyTo":null,"subject":"[PATCH] Verify index file before we opportunistically update it","fromName":"Yiannis Marangos","fromEmail":"yiannis.marangos@gmail.com","sentAt":"2014-04-09T22:06:36Z","receivedAt":"2014-04-09T22:06:36Z","isPatch":true,"sender":{"key":"yiannis.marangos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/194040?v=4"},"body":"This is a fix for the following bug:\nhttp://thread.gmane.org/gmane.comp.version-control.git/245946/focus=245965\n\nI added 2 functions: verify_index_from and verify_index. They return 1\nif the sha1 is correct, otherwise 0. I choose to not die if any errors\nare occurred because we just want to not proceed to \"opportunistic\nupdate\".\n\nSome questions:\n1) Is it better to have these functions as static?\n2) If the answer of (1) is no, should I define verify_cache*() also?\n3) If something goes wrong in verify_hdr(), it will print an error\n message, should I make a \"quietly\" version of it?\n\nYiannis Marangos (1):\n Verify index file before we opportunistically update it\n\n cache.h | 3 +++\n read-cache.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++---------\n 2 files changed, 71 insertions(+), 11 deletions(-)\n\n-- \n1.9.1\n"},{"id":"238607","messageId":"1397081197-14803-2-git-send-email-yiannis.marangos@gmail.com","threadId":"36385","inReplyTo":"1397081197-14803-1-git-send-email-yiannis.marangos@gmail.com","subject":"[PATCH] Verify index file before we opportunistically update it","fromName":"Yiannis Marangos","fromEmail":"yiannis.marangos@gmail.com","sentAt":"2014-04-09T22:06:37Z","receivedAt":"2014-04-09T22:06:37Z","isPatch":true,"sender":{"key":"yiannis.marangos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/194040?v=4"},"body":"Before we proceed to \"opportunistic update\" we must verify that the\ncurrent index file is the same as the one that we read before. There\nis a possible race if we don't do this:\n\n 1. process A calls git-rebase\n\n 2. process A applies 1st commit\n\n 3. process B calls git-status\n\n 4. process B reads index\n\n 5. process A applies 2nd commit\n\n 6. process B takes the lock, then overwrites process A's changes.\n\n 7. process A applies 3rd commit\n\nAs an end result the 3rd commit will have a revert of the 2nd commit.\n\nSigned-off-by: Yiannis Marangos \n---\n cache.h | 3 +++\n read-cache.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++---------\n 2 files changed, 71 insertions(+), 11 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 107ac61..b0eedad 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -279,6 +279,7 @@ struct index_state {\n \t\t initialized : 1;\n \tstruct hashmap name_hash;\n \tstruct hashmap dir_hash;\n+\tunsigned char sha1[20];\n };\n \n extern struct index_state the_index;\n@@ -456,6 +457,8 @@ extern int daemonize(void);\n \t} while (0)\n \n /* Initialize and use the cache information */\n+extern int verify_index_from(const struct index_state *, const char *path);\n+extern int verify_index(struct index_state *);\n extern int read_index(struct index_state *);\n extern int read_index_preload(struct index_state *, const struct pathspec *pathspec);\n extern int read_index_from(struct index_state *, const char *path);\ndiff --git a/read-cache.c b/read-cache.c\nindex ba13353..fac8655 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -14,6 +14,7 @@\n #include \"resolve-undo.h\"\n #include \"strbuf.h\"\n #include \"varint.h\"\n+#include \"git-compat-util.h\"\n \n static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,\n \t\t\t\t\t unsigned int options);\n@@ -1321,6 +1322,59 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)\n \treturn 0;\n }\n \n+/* This function verifies if index_state has the correct sha1 of an index file.\n+ * Don't die if we have any other failure, just return 0. */\n+int verify_index_from(const struct index_state *istate, const char *path)\n+{\n+\tint fd;\n+\tstruct stat st;\n+\tstruct cache_header *hdr;\n+\tvoid *mmap_addr;\n+\tsize_t mmap_size;\n+\n+\tif (!istate->initialized)\n+\t\treturn 0;\n+\n+\tfd = open(path, O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn 0;\n+\n+\tif (fstat(fd, &st))\n+\t\treturn 0;\n+\n+\t/* file is too big */\n+\tif (st.st_size > (size_t)st.st_size)\n+\t\treturn 0;\n+\n+\tmmap_size = (size_t)st.st_size;\n+\tif (mmap_size < sizeof(struct cache_header) + 20)\n+\t\treturn 0;\n+\n+\tmmap_addr = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n+\tclose(fd);\n+\tif (mmap_addr == MAP_FAILED)\n+\t\treturn 0;\n+\n+\thdr = mmap_addr;\n+\tif (verify_hdr(hdr, mmap_size) < 0)\n+\t\tgoto unmap;\n+\n+\tif (hashcmp(istate->sha1, (unsigned char *)hdr + mmap_size - 20))\n+\t\tgoto unmap;\n+\n+\tmunmap(mmap_addr, mmap_size);\n+\treturn 1;\n+\n+unmap:\n+\tmunmap(mmap_addr, mmap_size);\n+\treturn 0;\n+}\n+\n+int verify_index(struct index_state *istate)\n+{\n+\treturn verify_index_from(istate, get_index_file());\n+}\n+\n static int read_index_extension(struct index_state *istate,\n \t\t\t\tconst char *ext, void *data, unsigned long sz)\n {\n@@ -1445,7 +1499,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \tstruct stat st;\n \tunsigned long src_offset;\n \tstruct cache_header *hdr;\n-\tvoid *mmap;\n+\tvoid *mmap_addr;\n \tsize_t mmap_size;\n \tstruct strbuf previous_name_buf = STRBUF_INIT, *previous_name;\n \n@@ -1468,15 +1522,16 @@ int read_index_from(struct index_state *istate, const char *path)\n \tif (mmap_size < sizeof(struct cache_header) + 20)\n \t\tdie(\"index file smaller than expected\");\n \n-\tmmap = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n-\tif (mmap == MAP_FAILED)\n+\tmmap_addr = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n+\tif (mmap_addr == MAP_FAILED)\n \t\tdie_errno(\"unable to map index file\");\n \tclose(fd);\n \n-\thdr = mmap;\n+\thdr = mmap_addr;\n \tif (verify_hdr(hdr, mmap_size) < 0)\n \t\tgoto unmap;\n \n+\thashcpy(istate->sha1, (unsigned char *)hdr + mmap_size - 20);\n \tistate->version = ntohl(hdr->hdr_version);\n \tistate->cache_nr = ntohl(hdr->hdr_entries);\n \tistate->cache_alloc = alloc_nr(istate->cache_nr);\n@@ -1494,7 +1549,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \t\tstruct cache_entry *ce;\n \t\tunsigned long consumed;\n \n-\t\tdisk_ce = (struct ondisk_cache_entry *)((char *)mmap + src_offset);\n+\t\tdisk_ce = (struct ondisk_cache_entry *)((char *)mmap_addr + src_offset);\n \t\tce = create_from_disk(disk_ce, &consumed, previous_name);\n \t\tset_index_entry(istate, i, ce);\n \n@@ -1512,21 +1567,21 @@ int read_index_from(struct index_state *istate, const char *path)\n \t\t * in 4-byte network byte order.\n \t\t */\n \t\tuint32_t extsize;\n-\t\tmemcpy(&extsize, (char *)mmap + src_offset + 4, 4);\n+\t\tmemcpy(&extsize, (char *)mmap_addr + src_offset + 4, 4);\n \t\textsize = ntohl(extsize);\n \t\tif (read_index_extension(istate,\n-\t\t\t\t\t (const char *) mmap + src_offset,\n-\t\t\t\t\t (char *) mmap + src_offset + 8,\n+\t\t\t\t\t (const char *) mmap_addr + src_offset,\n+\t\t\t\t\t (char *) mmap_addr + src_offset + 8,\n \t\t\t\t\t extsize) < 0)\n \t\t\tgoto unmap;\n \t\tsrc_offset += 8;\n \t\tsrc_offset += extsize;\n \t}\n-\tmunmap(mmap, mmap_size);\n+\tmunmap(mmap_addr, mmap_size);\n \treturn istate->cache_nr;\n \n unmap:\n-\tmunmap(mmap, mmap_size);\n+\tmunmap(mmap_addr, mmap_size);\n \tdie(\"index file corrupt\");\n }\n \n@@ -1778,8 +1833,10 @@ static int has_racy_timestamp(struct index_state *istate)\n */\n void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)\n {\n+\tprintf(\"cache_changed: %d has_racy_timestamp: %d\\n\",\n+\t istate->cache_changed, has_racy_timestamp(istate));\n \tif ((istate->cache_changed || has_racy_timestamp(istate)) &&\n-\t !write_index(istate, lockfile->fd))\n+\t verify_index(istate) && !write_index(istate, lockfile->fd))\n \t\tcommit_locked_index(lockfile);\n \telse\n \t\trollback_lock_file(lockfile);\n-- \n1.9.1\n"},{"id":"238608","messageId":"1397082869-24873-1-git-send-email-yiannis.marangos@gmail.com","threadId":"36385","inReplyTo":"1397081197-14803-1-git-send-email-yiannis.marangos@gmail.com","subject":"[PATCH v2] Verify index file before we opportunistically update it","fromName":"Yiannis Marangos","fromEmail":"yiannis.marangos@gmail.com","sentAt":"2014-04-09T22:34:29Z","receivedAt":"2014-04-09T22:34:29Z","isPatch":true,"sender":{"key":"yiannis.marangos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/194040?v=4"},"body":"Before we proceed to \"opportunistic update\" we must verify that the\ncurrent index file is the same as the one that we read before. There\nis a possible race if we don't do this:\n\n 1. process A calls git-rebase\n\n 2. process A applies 1st commit\n\n 3. process B calls git-status\n\n 4. process B reads index\n\n 5. process A applies 2nd commit\n\n 6. process B takes the lock, then overwrites process A's changes.\n\n 7. process A applies 3rd commit\n\nAs an end result the 3rd commit will have a revert of the 2nd commit.\n\nSigned-off-by: Yiannis Marangos \n---\n cache.h | 3 +++\n read-cache.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++---------\n 2 files changed, 69 insertions(+), 11 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 107ac61..b0eedad 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -279,6 +279,7 @@ struct index_state {\n \t\t initialized : 1;\n \tstruct hashmap name_hash;\n \tstruct hashmap dir_hash;\n+\tunsigned char sha1[20];\n };\n \n extern struct index_state the_index;\n@@ -456,6 +457,8 @@ extern int daemonize(void);\n \t} while (0)\n \n /* Initialize and use the cache information */\n+extern int verify_index_from(const struct index_state *, const char *path);\n+extern int verify_index(struct index_state *);\n extern int read_index(struct index_state *);\n extern int read_index_preload(struct index_state *, const struct pathspec *pathspec);\n extern int read_index_from(struct index_state *, const char *path);\ndiff --git a/read-cache.c b/read-cache.c\nindex ba13353..a49a441 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -14,6 +14,7 @@\n #include \"resolve-undo.h\"\n #include \"strbuf.h\"\n #include \"varint.h\"\n+#include \"git-compat-util.h\"\n \n static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,\n \t\t\t\t\t unsigned int options);\n@@ -1321,6 +1322,59 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)\n \treturn 0;\n }\n \n+/* This function verifies if index_state has the correct sha1 of an index file.\n+ * Don't die if we have any other failure, just return 0. */\n+int verify_index_from(const struct index_state *istate, const char *path)\n+{\n+\tint fd;\n+\tstruct stat st;\n+\tstruct cache_header *hdr;\n+\tvoid *mmap_addr;\n+\tsize_t mmap_size;\n+\n+\tif (!istate->initialized)\n+\t\treturn 0;\n+\n+\tfd = open(path, O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn 0;\n+\n+\tif (fstat(fd, &st))\n+\t\treturn 0;\n+\n+\t/* file is too big */\n+\tif (st.st_size > (size_t)st.st_size)\n+\t\treturn 0;\n+\n+\tmmap_size = (size_t)st.st_size;\n+\tif (mmap_size < sizeof(struct cache_header) + 20)\n+\t\treturn 0;\n+\n+\tmmap_addr = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n+\tclose(fd);\n+\tif (mmap_addr == MAP_FAILED)\n+\t\treturn 0;\n+\n+\thdr = mmap_addr;\n+\tif (verify_hdr(hdr, mmap_size) < 0)\n+\t\tgoto unmap;\n+\n+\tif (hashcmp(istate->sha1, (unsigned char *)hdr + mmap_size - 20))\n+\t\tgoto unmap;\n+\n+\tmunmap(mmap_addr, mmap_size);\n+\treturn 1;\n+\n+unmap:\n+\tmunmap(mmap_addr, mmap_size);\n+\treturn 0;\n+}\n+\n+int verify_index(struct index_state *istate)\n+{\n+\treturn verify_index_from(istate, get_index_file());\n+}\n+\n static int read_index_extension(struct index_state *istate,\n \t\t\t\tconst char *ext, void *data, unsigned long sz)\n {\n@@ -1445,7 +1499,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \tstruct stat st;\n \tunsigned long src_offset;\n \tstruct cache_header *hdr;\n-\tvoid *mmap;\n+\tvoid *mmap_addr;\n \tsize_t mmap_size;\n \tstruct strbuf previous_name_buf = STRBUF_INIT, *previous_name;\n \n@@ -1468,15 +1522,16 @@ int read_index_from(struct index_state *istate, const char *path)\n \tif (mmap_size < sizeof(struct cache_header) + 20)\n \t\tdie(\"index file smaller than expected\");\n \n-\tmmap = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n-\tif (mmap == MAP_FAILED)\n+\tmmap_addr = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n+\tif (mmap_addr == MAP_FAILED)\n \t\tdie_errno(\"unable to map index file\");\n \tclose(fd);\n \n-\thdr = mmap;\n+\thdr = mmap_addr;\n \tif (verify_hdr(hdr, mmap_size) < 0)\n \t\tgoto unmap;\n \n+\thashcpy(istate->sha1, (unsigned char *)hdr + mmap_size - 20);\n \tistate->version = ntohl(hdr->hdr_version);\n \tistate->cache_nr = ntohl(hdr->hdr_entries);\n \tistate->cache_alloc = alloc_nr(istate->cache_nr);\n@@ -1494,7 +1549,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \t\tstruct cache_entry *ce;\n \t\tunsigned long consumed;\n \n-\t\tdisk_ce = (struct ondisk_cache_entry *)((char *)mmap + src_offset);\n+\t\tdisk_ce = (struct ondisk_cache_entry *)((char *)mmap_addr + src_offset);\n \t\tce = create_from_disk(disk_ce, &consumed, previous_name);\n \t\tset_index_entry(istate, i, ce);\n \n@@ -1512,21 +1567,21 @@ int read_index_from(struct index_state *istate, const char *path)\n \t\t * in 4-byte network byte order.\n \t\t */\n \t\tuint32_t extsize;\n-\t\tmemcpy(&extsize, (char *)mmap + src_offset + 4, 4);\n+\t\tmemcpy(&extsize, (char *)mmap_addr + src_offset + 4, 4);\n \t\textsize = ntohl(extsize);\n \t\tif (read_index_extension(istate,\n-\t\t\t\t\t (const char *) mmap + src_offset,\n-\t\t\t\t\t (char *) mmap + src_offset + 8,\n+\t\t\t\t\t (const char *) mmap_addr + src_offset,\n+\t\t\t\t\t (char *) mmap_addr + src_offset + 8,\n \t\t\t\t\t extsize) < 0)\n \t\t\tgoto unmap;\n \t\tsrc_offset += 8;\n \t\tsrc_offset += extsize;\n \t}\n-\tmunmap(mmap, mmap_size);\n+\tmunmap(mmap_addr, mmap_size);\n \treturn istate->cache_nr;\n \n unmap:\n-\tmunmap(mmap, mmap_size);\n+\tmunmap(mmap_addr, mmap_size);\n \tdie(\"index file corrupt\");\n }\n \n@@ -1779,7 +1834,7 @@ static int has_racy_timestamp(struct index_state *istate)\n void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)\n {\n \tif ((istate->cache_changed || has_racy_timestamp(istate)) &&\n-\t !write_index(istate, lockfile->fd))\n+\t verify_index(istate) && !write_index(istate, lockfile->fd))\n \t\tcommit_locked_index(lockfile);\n \telse\n \t\trollback_lock_file(lockfile);\n-- \n1.9.1\n"},{"id":"238609","messageId":"1397083967-30401-1-git-send-email-yiannis.marangos@gmail.com","threadId":"36385","inReplyTo":"1397081197-14803-1-git-send-email-yiannis.marangos@gmail.com","subject":"[PATCH v3] Verify index file before we opportunistically update it","fromName":"Yiannis Marangos","fromEmail":"yiannis.marangos@gmail.com","sentAt":"2014-04-09T22:52:47Z","receivedAt":"2014-04-09T22:52:47Z","isPatch":true,"sender":{"key":"yiannis.marangos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/194040?v=4"},"body":"Before we proceed to \"opportunistic update\" we must verify that the\ncurrent index file is the same as the one that we read before. There\nis a possible race if we don't do this:\n\n 1. process A calls git-rebase\n\n 2. process A applies 1st commit\n\n 3. process B calls git-status\n\n 4. process B reads index\n\n 5. process A applies 2nd commit\n\n 6. process B takes the lock, then overwrites process A's changes.\n\n 7. process A applies 3rd commit\n\nAs an end result the 3rd commit will have a revert of the 2nd commit.\n\nSigned-off-by: Yiannis Marangos \n---\n cache.h | 3 +++\n read-cache.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++---------\n 2 files changed, 69 insertions(+), 11 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 107ac61..76ce3d9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -279,6 +279,7 @@ struct index_state {\n \t\t initialized : 1;\n \tstruct hashmap name_hash;\n \tstruct hashmap dir_hash;\n+\tunsigned char sha1[20];\n };\n \n extern struct index_state the_index;\n@@ -456,6 +457,8 @@ extern int daemonize(void);\n \t} while (0)\n \n /* Initialize and use the cache information */\n+extern int verify_index_from(const struct index_state *, const char *path);\n+extern int verify_index(const struct index_state *);\n extern int read_index(struct index_state *);\n extern int read_index_preload(struct index_state *, const struct pathspec *pathspec);\n extern int read_index_from(struct index_state *, const char *path);\ndiff --git a/read-cache.c b/read-cache.c\nindex ba13353..3604d8c 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -14,6 +14,7 @@\n #include \"resolve-undo.h\"\n #include \"strbuf.h\"\n #include \"varint.h\"\n+#include \"git-compat-util.h\"\n \n static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,\n \t\t\t\t\t unsigned int options);\n@@ -1321,6 +1322,59 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)\n \treturn 0;\n }\n \n+/* This function verifies if index_state has the correct sha1 of an index file.\n+ * Don't die if we have any other failure, just return 0. */\n+int verify_index_from(const struct index_state *istate, const char *path)\n+{\n+\tint fd;\n+\tstruct stat st;\n+\tstruct cache_header *hdr;\n+\tvoid *mmap_addr;\n+\tsize_t mmap_size;\n+\n+\tif (!istate->initialized)\n+\t\treturn 0;\n+\n+\tfd = open(path, O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn 0;\n+\n+\tif (fstat(fd, &st))\n+\t\treturn 0;\n+\n+\t/* file is too big */\n+\tif (st.st_size > (size_t)st.st_size)\n+\t\treturn 0;\n+\n+\tmmap_size = (size_t)st.st_size;\n+\tif (mmap_size < sizeof(struct cache_header) + 20)\n+\t\treturn 0;\n+\n+\tmmap_addr = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n+\tclose(fd);\n+\tif (mmap_addr == MAP_FAILED)\n+\t\treturn 0;\n+\n+\thdr = mmap_addr;\n+\tif (verify_hdr(hdr, mmap_size) < 0)\n+\t\tgoto unmap;\n+\n+\tif (hashcmp(istate->sha1, (unsigned char *)hdr + mmap_size - 20))\n+\t\tgoto unmap;\n+\n+\tmunmap(mmap_addr, mmap_size);\n+\treturn 1;\n+\n+unmap:\n+\tmunmap(mmap_addr, mmap_size);\n+\treturn 0;\n+}\n+\n+int verify_index(const struct index_state *istate)\n+{\n+\treturn verify_index_from(istate, get_index_file());\n+}\n+\n static int read_index_extension(struct index_state *istate,\n \t\t\t\tconst char *ext, void *data, unsigned long sz)\n {\n@@ -1445,7 +1499,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \tstruct stat st;\n \tunsigned long src_offset;\n \tstruct cache_header *hdr;\n-\tvoid *mmap;\n+\tvoid *mmap_addr;\n \tsize_t mmap_size;\n \tstruct strbuf previous_name_buf = STRBUF_INIT, *previous_name;\n \n@@ -1468,15 +1522,16 @@ int read_index_from(struct index_state *istate, const char *path)\n \tif (mmap_size < sizeof(struct cache_header) + 20)\n \t\tdie(\"index file smaller than expected\");\n \n-\tmmap = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n-\tif (mmap == MAP_FAILED)\n+\tmmap_addr = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n+\tif (mmap_addr == MAP_FAILED)\n \t\tdie_errno(\"unable to map index file\");\n \tclose(fd);\n \n-\thdr = mmap;\n+\thdr = mmap_addr;\n \tif (verify_hdr(hdr, mmap_size) < 0)\n \t\tgoto unmap;\n \n+\thashcpy(istate->sha1, (unsigned char *)hdr + mmap_size - 20);\n \tistate->version = ntohl(hdr->hdr_version);\n \tistate->cache_nr = ntohl(hdr->hdr_entries);\n \tistate->cache_alloc = alloc_nr(istate->cache_nr);\n@@ -1494,7 +1549,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \t\tstruct cache_entry *ce;\n \t\tunsigned long consumed;\n \n-\t\tdisk_ce = (struct ondisk_cache_entry *)((char *)mmap + src_offset);\n+\t\tdisk_ce = (struct ondisk_cache_entry *)((char *)mmap_addr + src_offset);\n \t\tce = create_from_disk(disk_ce, &consumed, previous_name);\n \t\tset_index_entry(istate, i, ce);\n \n@@ -1512,21 +1567,21 @@ int read_index_from(struct index_state *istate, const char *path)\n \t\t * in 4-byte network byte order.\n \t\t */\n \t\tuint32_t extsize;\n-\t\tmemcpy(&extsize, (char *)mmap + src_offset + 4, 4);\n+\t\tmemcpy(&extsize, (char *)mmap_addr + src_offset + 4, 4);\n \t\textsize = ntohl(extsize);\n \t\tif (read_index_extension(istate,\n-\t\t\t\t\t (const char *) mmap + src_offset,\n-\t\t\t\t\t (char *) mmap + src_offset + 8,\n+\t\t\t\t\t (const char *) mmap_addr + src_offset,\n+\t\t\t\t\t (char *) mmap_addr + src_offset + 8,\n \t\t\t\t\t extsize) < 0)\n \t\t\tgoto unmap;\n \t\tsrc_offset += 8;\n \t\tsrc_offset += extsize;\n \t}\n-\tmunmap(mmap, mmap_size);\n+\tmunmap(mmap_addr, mmap_size);\n \treturn istate->cache_nr;\n \n unmap:\n-\tmunmap(mmap, mmap_size);\n+\tmunmap(mmap_addr, mmap_size);\n \tdie(\"index file corrupt\");\n }\n \n@@ -1779,7 +1834,7 @@ static int has_racy_timestamp(struct index_state *istate)\n void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)\n {\n \tif ((istate->cache_changed || has_racy_timestamp(istate)) &&\n-\t !write_index(istate, lockfile->fd))\n+\t verify_index(istate) && !write_index(istate, lockfile->fd))\n \t\tcommit_locked_index(lockfile);\n \telse\n \t\trollback_lock_file(lockfile);\n-- \n1.9.1\n"},{"id":"238610","messageId":"xmqqwqeyxfwi.fsf@gitster.dls.corp.google.com","threadId":"36385","inReplyTo":"1397082869-24873-1-git-send-email-yiannis.marangos@gmail.com","subject":"Re: [PATCH v2] Verify index file before we opportunistically update it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-09T23:05:33Z","receivedAt":"2014-04-09T23:05:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yiannis Marangos writes:\n\n> Before we proceed to \"opportunistic update\" we must verify that the\n> current index file is the same as the one that we read before. There\n> is a possible race if we don't do this:\n\nPlease somehow make it clear that the race is in general, and use of\n\"git rebase\" in this description is merely an example.\n\n> 1. process A calls git-rebase\n\n... or does anything that uses the index.\n\n> 2. process A applies 1st commit\n>\n> 3. process B calls git-status\n>\n> 4. process B reads index\n>\n> 5. process A applies 2nd commit\n\n... or does anything that updates the index.\n\n> 6. process B takes the lock, then overwrites process A's changes.\n>\n> 7. process A applies 3rd commit\n>\n> As an end result the 3rd commit will have a revert of the 2nd commit.\n\nAnd the missing piece is...? \"When process B takes the lock, it\nneeds to make sure that the index hasn't changed since it read at\nstep 4.\"\n\n> Signed-off-by: Yiannis Marangos \n> ---\n\nThis is a place for you to describe how this version is different\nfrom the previous round. I am guessing that the only change is that\nyou removed the unnecessary printf() from the top of if-able, but I\ndidn't compare (nor read either versions carefully).\n\n> cache.h | 3 +++\n> read-cache.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++---------\n> 2 files changed, 69 insertions(+), 11 deletions(-)\n>\n> diff --git a/cache.h b/cache.h\n> index 107ac61..b0eedad 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -279,6 +279,7 @@ struct index_state {\n> \t\t initialized : 1;\n> \tstruct hashmap name_hash;\n> \tstruct hashmap dir_hash;\n> +\tunsigned char sha1[20];\n> };\n> \n> extern struct index_state the_index;\n> @@ -456,6 +457,8 @@ extern int daemonize(void);\n> \t} while (0)\n> \n> /* Initialize and use the cache information */\n> +extern int verify_index_from(const struct index_state *, const char *path);\n> +extern int verify_index(struct index_state *);\n\nI do not think these should be extern; they are implementation\ndetails of the opportunistic update, and update-if-able is the only\nthing the outside world needs to know about, I think.\n\n> extern int read_index(struct index_state *);\n> extern int read_index_preload(struct index_state *, const struct pathspec *pathspec);\n> extern int read_index_from(struct index_state *, const char *path);\n> diff --git a/read-cache.c b/read-cache.c\n> index ba13353..a49a441 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -14,6 +14,7 @@\n> #include \"resolve-undo.h\"\n> #include \"strbuf.h\"\n> #include \"varint.h\"\n> +#include \"git-compat-util.h\"\n\nAn unnecessary change; we include \"cache.h\" as the very first thing\nin this file.\n\n> @@ -1321,6 +1322,59 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)\n> \treturn 0;\n> }\n> \n> +/* This function verifies if index_state has the correct sha1 of an index file.\n> + * Don't die if we have any other failure, just return 0. */\n\n\t/*\n * Please write multi-line comment\n * like this.\n */\n\n> +int verify_index_from(const struct index_state *istate, const char *path)\n> +{\n\nAlso, this does not have to be extern. At least not yet.\n\n> +\tint fd;\n> +\tstruct stat st;\n> +\tstruct cache_header *hdr;\n> +\tvoid *mmap_addr;\n> +\tsize_t mmap_size;\n> +\n> +\tif (!istate->initialized)\n> +\t\treturn 0;\n> +\n> +\tfd = open(path, O_RDONLY);\n> +\tif (fd < 0)\n> +\t\treturn 0;\n> +\n> +\tif (fstat(fd, &st))\n> +\t\treturn 0;\n> +\n> +\t/* file is too big */\n> +\tif (st.st_size > (size_t)st.st_size)\n> +\t\treturn 0;\n> +\n> +\tmmap_size = (size_t)st.st_size;\n> +\tif (mmap_size < sizeof(struct cache_header) + 20)\n> +\t\treturn 0;\n> +\n> +\tmmap_addr = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n\nWhy PROT_WRITE?\n\n> +\tclose(fd);\n> +\tif (mmap_addr == MAP_FAILED)\n> +\t\treturn 0;\n> +\n> +\thdr = mmap_addr;\n> +\tif (verify_hdr(hdr, mmap_size) < 0)\n> +\t\tgoto unmap;\n> +\n> +\tif (hashcmp(istate->sha1, (unsigned char *)hdr + mmap_size - 20))\n> +\t\tgoto unmap;\n> +\n> +\tmunmap(mmap_addr, mmap_size);\n> +\treturn 1;\n> +\n> +unmap:\n> +\tmunmap(mmap_addr, mmap_size);\n> +\treturn 0;\n> +}\n> +\n> +int verify_index(struct index_state *istate)\n> +{\n> +\treturn verify_index_from(istate, get_index_file());\n> +}\n> +\n> static int read_index_extension(struct index_state *istate,\n> \t\t\t\tconst char *ext, void *data, unsigned long sz)\n> {\n> @@ -1445,7 +1499,7 @@ int read_index_from(struct index_state *istate, const char *path)\n> \tstruct stat st;\n> \tunsigned long src_offset;\n> \tstruct cache_header *hdr;\n> -\tvoid *mmap;\n> +\tvoid *mmap_addr;\n\nThis introduces noise to the patch to make it harder to review, and\ndoes not look a necessary change. Am I mistaken?\n\n> \tsize_t mmap_size;\n> \tstruct strbuf previous_name_buf = STRBUF_INIT, *previous_name;\n> \n> @@ -1468,15 +1522,16 @@ int read_index_from(struct index_state *istate, const char *path)\n> \tif (mmap_size < sizeof(struct cache_header) + 20)\n> \t\tdie(\"index file smaller than expected\");\n> \n> -\tmmap = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n> -\tif (mmap == MAP_FAILED)\n> +\tmmap_addr = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n> +\tif (mmap_addr == MAP_FAILED)\n> \t\tdie_errno(\"unable to map index file\");\n> \tclose(fd);\n> \n> -\thdr = mmap;\n> +\thdr = mmap_addr;\n> \tif (verify_hdr(hdr, mmap_size) < 0)\n> \t\tgoto unmap;\n> \n> +\thashcpy(istate->sha1, (unsigned char *)hdr + mmap_size - 20);\n> \tistate->version = ntohl(hdr->hdr_version);\n> \tistate->cache_nr = ntohl(hdr->hdr_entries);\n> \tistate->cache_alloc = alloc_nr(istate->cache_nr);\n> @@ -1494,7 +1549,7 @@ int read_index_from(struct index_state *istate, const char *path)\n> \t\tstruct cache_entry *ce;\n> \t\tunsigned long consumed;\n> \n> -\t\tdisk_ce = (struct ondisk_cache_entry *)((char *)mmap + src_offset);\n> +\t\tdisk_ce = (struct ondisk_cache_entry *)((char *)mmap_addr + src_offset);\n> \t\tce = create_from_disk(disk_ce, &consumed, previous_name);\n> \t\tset_index_entry(istate, i, ce);\n> \n> @@ -1512,21 +1567,21 @@ int read_index_from(struct index_state *istate, const char *path)\n> \t\t * in 4-byte network byte order.\n> \t\t */\n> \t\tuint32_t extsize;\n> -\t\tmemcpy(&extsize, (char *)mmap + src_offset + 4, 4);\n> +\t\tmemcpy(&extsize, (char *)mmap_addr + src_offset + 4, 4);\n> \t\textsize = ntohl(extsize);\n> \t\tif (read_index_extension(istate,\n> -\t\t\t\t\t (const char *) mmap + src_offset,\n> -\t\t\t\t\t (char *) mmap + src_offset + 8,\n> +\t\t\t\t\t (const char *) mmap_addr + src_offset,\n> +\t\t\t\t\t (char *) mmap_addr + src_offset + 8,\n> \t\t\t\t\t extsize) < 0)\n> \t\t\tgoto unmap;\n> \t\tsrc_offset += 8;\n> \t\tsrc_offset += extsize;\n> \t}\n> -\tmunmap(mmap, mmap_size);\n> +\tmunmap(mmap_addr, mmap_size);\n> \treturn istate->cache_nr;\n> \n> unmap:\n> -\tmunmap(mmap, mmap_size);\n> +\tmunmap(mmap_addr, mmap_size);\n> \tdie(\"index file corrupt\");\n> }\n> \n> @@ -1779,7 +1834,7 @@ static int has_racy_timestamp(struct index_state *istate)\n> void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)\n> {\n> \tif ((istate->cache_changed || has_racy_timestamp(istate)) &&\n> -\t !write_index(istate, lockfile->fd))\n> +\t verify_index(istate) && !write_index(istate, lockfile->fd))\n> \t\tcommit_locked_index(lockfile);\n> \telse\n> \t\trollback_lock_file(lockfile);\n"},{"id":"238614","messageId":"1397107367-29974-1-git-send-email-yiannis.marangos@gmail.com","threadId":"36385","inReplyTo":"1397081197-14803-1-git-send-email-yiannis.marangos@gmail.com","subject":"[PATCH v4] Verify index file before we opportunistically update it","fromName":"Yiannis Marangos","fromEmail":"yiannis.marangos@gmail.com","sentAt":"2014-04-10T05:22:47Z","receivedAt":"2014-04-10T05:22:47Z","isPatch":true,"sender":{"key":"yiannis.marangos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/194040?v=4"},"body":"Before we proceed to \"opportunistic update\" we must verify that the\ncurrent index file is the same as the one that we read before. There\nis a possible race if we don't do this. In the example below git-show\ndoes \"opportunistic update\" and git-rebase updates the index, but the\nrace can happen in general.\n\n 1. process A calls git-rebase (or does anything that uses the index)\n\n 2. process A applies 1st commit\n\n 3. process B calls git-status (or does anything that updates the index)\n\n 4. process B reads index\n\n 5. process A applies 2nd commit\n\n 6. process B takes the lock, then overwrites process A's changes.\n\n 7. process A applies 3rd commit\n\nAs an end result the 3rd commit will have a revert of the 2nd commit.\nWhen process B takes the lock, it needs to make sure that the index\nhasn't changed since step 4.\n\nSigned-off-by: Yiannis Marangos \n---\n\nVersion 4 contains fixes based on Junio's comments.\n\n cache.h | 1 +\n read-cache.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 58 insertions(+), 1 deletion(-)\n\ndiff --git a/cache.h b/cache.h\nindex 107ac61..0460f06 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -279,6 +279,7 @@ struct index_state {\n \t\t initialized : 1;\n \tstruct hashmap name_hash;\n \tstruct hashmap dir_hash;\n+\tunsigned char sha1[20];\n };\n \n extern struct index_state the_index;\ndiff --git a/read-cache.c b/read-cache.c\nindex ba13353..73a15b1 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1477,6 +1477,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \tif (verify_hdr(hdr, mmap_size) < 0)\n \t\tgoto unmap;\n \n+\thashcpy(istate->sha1, (unsigned char *)hdr + mmap_size - 20);\n \tistate->version = ntohl(hdr->hdr_version);\n \tistate->cache_nr = ntohl(hdr->hdr_entries);\n \tistate->cache_alloc = alloc_nr(istate->cache_nr);\n@@ -1760,6 +1761,61 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,\n \treturn result;\n }\n \n+/*\n+ * This function verifies if index_state has the correct sha1 of an index file.\n+ * Don't die if we have any other failure, just return 0.\n+ */\n+static int verify_index_from(const struct index_state *istate, const char *path)\n+{\n+\tint fd;\n+\tstruct stat st;\n+\tstruct cache_header *hdr;\n+\tvoid *mmap_addr;\n+\tsize_t mmap_size;\n+\n+\tif (!istate->initialized)\n+\t\treturn 0;\n+\n+\tfd = open(path, O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn 0;\n+\n+\tif (fstat(fd, &st))\n+\t\treturn 0;\n+\n+\t/* file is too big */\n+\tif (st.st_size > (size_t)st.st_size)\n+\t\treturn 0;\n+\n+\tmmap_size = (size_t)st.st_size;\n+\tif (mmap_size < sizeof(struct cache_header) + 20)\n+\t\treturn 0;\n+\n+\tmmap_addr = mmap(NULL, mmap_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tclose(fd);\n+\tif (mmap_addr == MAP_FAILED)\n+\t\treturn 0;\n+\n+\thdr = mmap_addr;\n+\tif (verify_hdr(hdr, mmap_size) < 0)\n+\t\tgoto unmap;\n+\n+\tif (hashcmp(istate->sha1, (unsigned char *)hdr + mmap_size - 20))\n+\t\tgoto unmap;\n+\n+\tmunmap(mmap_addr, mmap_size);\n+\treturn 1;\n+\n+unmap:\n+\tmunmap(mmap_addr, mmap_size);\n+\treturn 0;\n+}\n+\n+static int verify_index(const struct index_state *istate)\n+{\n+\treturn verify_index_from(istate, get_index_file());\n+}\n+\n static int has_racy_timestamp(struct index_state *istate)\n {\n \tint entries = istate->cache_nr;\n@@ -1779,7 +1835,7 @@ static int has_racy_timestamp(struct index_state *istate)\n void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)\n {\n \tif ((istate->cache_changed || has_racy_timestamp(istate)) &&\n-\t !write_index(istate, lockfile->fd))\n+\t verify_index(istate) && !write_index(istate, lockfile->fd))\n \t\tcommit_locked_index(lockfile);\n \telse\n \t\trollback_lock_file(lockfile);\n-- \n1.9.1\n"},{"id":"238615","messageId":"1397108075-30891-1-git-send-email-yiannis.marangos@gmail.com","threadId":"36385","inReplyTo":"1397081197-14803-1-git-send-email-yiannis.marangos@gmail.com","subject":"[PATCH v5] Verify index file before we opportunistically update it","fromName":"Yiannis Marangos","fromEmail":"yiannis.marangos@gmail.com","sentAt":"2014-04-10T05:34:35Z","receivedAt":"2014-04-10T05:34:35Z","isPatch":true,"sender":{"key":"yiannis.marangos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/194040?v=4"},"body":"Before we proceed to \"opportunistic update\" we must verify that the\ncurrent index file is the same as the one that we read before. There\nis a possible race if we don't do this. In the example below git-status\ndoes \"opportunistic update\" and git-rebase updates the index, but the\nrace can happen in general.\n\n 1. process A calls git-rebase (or does anything that uses the index)\n\n 2. process A applies 1st commit\n\n 3. process B calls git-status (or does anything that updates the index)\n\n 4. process B reads index\n\n 5. process A applies 2nd commit\n\n 6. process B takes the lock, then overwrites process A's changes.\n\n 7. process A applies 3rd commit\n\nAs an end result the 3rd commit will have a revert of the 2nd commit.\nWhen process B takes the lock, it needs to make sure that the index\nhasn't changed since step 4.\n\nSigned-off-by: Yiannis Marangos \n---\n\nVersion 4 contains fixes based on Junio's comments.\nVersion 5 fixs a typo in commit message (git-show -> git-status)\n\n cache.h | 1 +\n read-cache.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 58 insertions(+), 1 deletion(-)\n\ndiff --git a/cache.h b/cache.h\nindex 107ac61..0460f06 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -279,6 +279,7 @@ struct index_state {\n \t\t initialized : 1;\n \tstruct hashmap name_hash;\n \tstruct hashmap dir_hash;\n+\tunsigned char sha1[20];\n };\n \n extern struct index_state the_index;\ndiff --git a/read-cache.c b/read-cache.c\nindex ba13353..73a15b1 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1477,6 +1477,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \tif (verify_hdr(hdr, mmap_size) < 0)\n \t\tgoto unmap;\n \n+\thashcpy(istate->sha1, (unsigned char *)hdr + mmap_size - 20);\n \tistate->version = ntohl(hdr->hdr_version);\n \tistate->cache_nr = ntohl(hdr->hdr_entries);\n \tistate->cache_alloc = alloc_nr(istate->cache_nr);\n@@ -1760,6 +1761,61 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,\n \treturn result;\n }\n \n+/*\n+ * This function verifies if index_state has the correct sha1 of an index file.\n+ * Don't die if we have any other failure, just return 0.\n+ */\n+static int verify_index_from(const struct index_state *istate, const char *path)\n+{\n+\tint fd;\n+\tstruct stat st;\n+\tstruct cache_header *hdr;\n+\tvoid *mmap_addr;\n+\tsize_t mmap_size;\n+\n+\tif (!istate->initialized)\n+\t\treturn 0;\n+\n+\tfd = open(path, O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn 0;\n+\n+\tif (fstat(fd, &st))\n+\t\treturn 0;\n+\n+\t/* file is too big */\n+\tif (st.st_size > (size_t)st.st_size)\n+\t\treturn 0;\n+\n+\tmmap_size = (size_t)st.st_size;\n+\tif (mmap_size < sizeof(struct cache_header) + 20)\n+\t\treturn 0;\n+\n+\tmmap_addr = mmap(NULL, mmap_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tclose(fd);\n+\tif (mmap_addr == MAP_FAILED)\n+\t\treturn 0;\n+\n+\thdr = mmap_addr;\n+\tif (verify_hdr(hdr, mmap_size) < 0)\n+\t\tgoto unmap;\n+\n+\tif (hashcmp(istate->sha1, (unsigned char *)hdr + mmap_size - 20))\n+\t\tgoto unmap;\n+\n+\tmunmap(mmap_addr, mmap_size);\n+\treturn 1;\n+\n+unmap:\n+\tmunmap(mmap_addr, mmap_size);\n+\treturn 0;\n+}\n+\n+static int verify_index(const struct index_state *istate)\n+{\n+\treturn verify_index_from(istate, get_index_file());\n+}\n+\n static int has_racy_timestamp(struct index_state *istate)\n {\n \tint entries = istate->cache_nr;\n@@ -1779,7 +1835,7 @@ static int has_racy_timestamp(struct index_state *istate)\n void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)\n {\n \tif ((istate->cache_changed || has_racy_timestamp(istate)) &&\n-\t !write_index(istate, lockfile->fd))\n+\t verify_index(istate) && !write_index(istate, lockfile->fd))\n \t\tcommit_locked_index(lockfile);\n \telse\n \t\trollback_lock_file(lockfile);\n-- \n1.9.1\n"},{"id":"238619","messageId":"CACsJy8D9dsxOA6xVmhxa7N20OxEQ1a63rQm6QCtwq9ThG6E3RA@mail.gmail.com","threadId":"36385","inReplyTo":"1397108075-30891-1-git-send-email-yiannis.marangos@gmail.com","subject":"Re: [PATCH v5] Verify index file before we opportunistically update it","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-04-10T10:40:55Z","receivedAt":"2014-04-10T10:40:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Apr 10, 2014 at 12:34 PM, Yiannis Marangos\n wrote:\n> +/*\n> + * This function verifies if index_state has the correct sha1 of an index file.\n> + * Don't die if we have any other failure, just return 0.\n> + */\n> +static int verify_index_from(const struct index_state *istate, const char *path)\n> +{\n> + int fd;\n> + struct stat st;\n> + struct cache_header *hdr;\n> + void *mmap_addr;\n> + size_t mmap_size;\n> +\n> + if (!istate->initialized)\n> + return 0;\n> +\n> + fd = open(path, O_RDONLY);\n> + if (fd < 0)\n> + return 0;\n> +\n> + if (fstat(fd, &st))\n> + return 0;\n> +\n> + /* file is too big */\n> + if (st.st_size > (size_t)st.st_size)\n> + return 0;\n> +\n> + mmap_size = (size_t)st.st_size;\n> + if (mmap_size < sizeof(struct cache_header) + 20)\n> + return 0;\n> +\n> + mmap_addr = mmap(NULL, mmap_size, PROT_READ, MAP_PRIVATE, fd, 0);\n> + close(fd);\n> + if (mmap_addr == MAP_FAILED)\n> + return 0;\n> +\n> + hdr = mmap_addr;\n> + if (verify_hdr(hdr, mmap_size) < 0)\n> + goto unmap;\n\nverify_hdr() is a bit expensive because you need to digest the whole\nindex file (could big as big as 14MB on webkit). Could we get away\nwithout it? I mean, is it enough that we pick the last 20 bytes and\ncompare it with istate->sha1? If we only need 20 bytes, pread() may be\nbetter than mmap().\n\nThe chance of SHA-1 collision is small enough for us to ignore, I\nthink. And if a client updates the index without updating the trailing\nsha-1, the index is broken and we don't have to worry about\noverwriting it.\n\n> +\n> + if (hashcmp(istate->sha1, (unsigned char *)hdr + mmap_size - 20))\n> + goto unmap;\n> +\n> + munmap(mmap_addr, mmap_size);\n> + return 1;\n> +\n> +unmap:\n> + munmap(mmap_addr, mmap_size);\n> + return 0;\n> +}\n-- \nDuy\n"},{"id":"238620","messageId":"20140410115743.GA1446@abyss.utx.wifi","threadId":"36385","inReplyTo":"CACsJy8D9dsxOA6xVmhxa7N20OxEQ1a63rQm6QCtwq9ThG6E3RA@mail.gmail.com","subject":"Re: [PATCH v5] Verify index file before we opportunistically update it","fromName":"Yiannis Marangos","fromEmail":"yiannis.marangos@gmail.com","sentAt":"2014-04-10T11:57:43Z","receivedAt":"2014-04-10T11:57:43Z","isPatch":true,"sender":{"key":"yiannis.marangos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/194040?v=4"},"body":"On Thu, Apr 10, 2014 at 05:40:55PM +0700, Duy Nguyen wrote:\n> verify_hdr() is a bit expensive because you need to digest the whole\n> index file (could big as big as 14MB on webkit). Could we get away\n> without it? I mean, is it enough that we pick the last 20 bytes and\n> compare it with istate->sha1? If we only need 20 bytes, pread() may be\n> better than mmap().\n> \n> The chance of SHA-1 collision is small enough for us to ignore, I\n> think. And if a client updates the index without updating the trailing\n> sha-1, the index is broken and we don't have to worry about\n> overwriting it.\n\nThat's true, I will commit version 6 in a bit.\n"},{"id":"238621","messageId":"1397135502-9666-1-git-send-email-yiannis.marangos@gmail.com","threadId":"36385","inReplyTo":"1397081197-14803-1-git-send-email-yiannis.marangos@gmail.com","subject":"[PATCH v6] Verify index file before we opportunistically update it","fromName":"Yiannis Marangos","fromEmail":"yiannis.marangos@gmail.com","sentAt":"2014-04-10T13:11:42Z","receivedAt":"2014-04-10T13:11:42Z","isPatch":true,"sender":{"key":"yiannis.marangos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/194040?v=4"},"body":"Before we proceed to \"opportunistic update\" we must verify that the\ncurrent index file is the same as the one that we read before. There\nis a possible race if we don't do this. In the example below git-status\ndoes \"opportunistic update\" and git-rebase updates the index, but the\nrace can happen in general.\n\n 1. process A calls git-rebase (or does anything that uses the index)\n\n 2. process A applies 1st commit\n\n 3. process B calls git-status (or does anything that updates the index)\n\n 4. process B reads index\n\n 5. process A applies 2nd commit\n\n 6. process B takes the lock, then overwrites process A's changes.\n\n 7. process A applies 3rd commit\n\nAs an end result the 3rd commit will have a revert of the 2nd commit.\nWhen process B takes the lock, it needs to make sure that the index\nhasn't changed since step 4.\n\nSigned-off-by: Yiannis Marangos \n---\n\nVersion 4 contains fixes based on Junio's comments.\nVersion 5 fixs a typo in commit message (git-show -> git-status)\nVersion 6 removes verify_hdr() and use pread() instead of mmap().\n\n cache.h | 1 +\n read-cache.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 47 insertions(+), 1 deletion(-)\n\ndiff --git a/cache.h b/cache.h\nindex 107ac61..0460f06 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -279,6 +279,7 @@ struct index_state {\n \t\t initialized : 1;\n \tstruct hashmap name_hash;\n \tstruct hashmap dir_hash;\n+\tunsigned char sha1[20];\n };\n \n extern struct index_state the_index;\ndiff --git a/read-cache.c b/read-cache.c\nindex ba13353..034865d 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1477,6 +1477,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \tif (verify_hdr(hdr, mmap_size) < 0)\n \t\tgoto unmap;\n \n+\thashcpy(istate->sha1, (unsigned char *)hdr + mmap_size - 20);\n \tistate->version = ntohl(hdr->hdr_version);\n \tistate->cache_nr = ntohl(hdr->hdr_entries);\n \tistate->cache_alloc = alloc_nr(istate->cache_nr);\n@@ -1760,6 +1761,50 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,\n \treturn result;\n }\n \n+/*\n+ * This function verifies if index_state has the correct sha1 of an index file.\n+ * Don't die if we have any other failure, just return 0.\n+ */\n+static int verify_index_from(const struct index_state *istate, const char *path)\n+{\n+\tint fd;\n+\tssize_t n;\n+\tstruct stat st;\n+\tunsigned char sha1[20];\n+\n+\tif (!istate->initialized)\n+\t\treturn 0;\n+\n+\tfd = open(path, O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn 0;\n+\n+\tif (fstat(fd, &st))\n+\t\tgoto out;\n+\n+\tif (st.st_size < sizeof(struct cache_header) + 20)\n+\t\tgoto out;\n+\n+\tn = pread(fd, sha1, 20, st.st_size - 20);\n+\tif (n != 20)\n+\t\tgoto out;\n+\n+\tif (hashcmp(istate->sha1, sha1))\n+\t\tgoto out;\n+\n+\tclose(fd);\n+\treturn 1;\n+\n+out:\n+\tclose(fd);\n+\treturn 0;\n+}\n+\n+static int verify_index(const struct index_state *istate)\n+{\n+\treturn verify_index_from(istate, get_index_file());\n+}\n+\n static int has_racy_timestamp(struct index_state *istate)\n {\n \tint entries = istate->cache_nr;\n@@ -1779,7 +1824,7 @@ static int has_racy_timestamp(struct index_state *istate)\n void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)\n {\n \tif ((istate->cache_changed || has_racy_timestamp(istate)) &&\n-\t !write_index(istate, lockfile->fd))\n+\t verify_index(istate) && !write_index(istate, lockfile->fd))\n \t\tcommit_locked_index(lockfile);\n \telse\n \t\trollback_lock_file(lockfile);\n-- \n1.9.1\n"},{"id":"238622","messageId":"xmqqob09xgua.fsf@gitster.dls.corp.google.com","threadId":"36385","inReplyTo":"CACsJy8D9dsxOA6xVmhxa7N20OxEQ1a63rQm6QCtwq9ThG6E3RA@mail.gmail.com","subject":"Re: [PATCH v5] Verify index file before we opportunistically update it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-10T16:57:33Z","receivedAt":"2014-04-10T16:57:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> verify_hdr() is a bit expensive because you need to digest the whole\n> index file (could big as big as 14MB on webkit). Could we get away\n> without it? I mean, is it enough that we pick the last 20 bytes and\n> compare it with istate->sha1? If we only need 20 bytes, pread() may be\n> better than mmap().\n\nTrue.\n\nI do not think we offer xread() equivalent for pread() to help\ncoders to avoid the common mistake of failing to resume interrupted\nsystem calls, though.\n\nThe only callsite of pread() we currently have is in index-pack.c,\nwhich just does\n\n\t\tn = pread(pack_fd, inbuf, n, from);\n\t\tif (n < 0)\n\t\t\tdie_errno(_(\"cannot pread pack file\"));\n\nwithout paying attention to EAGAIN/EINTR, that seems wrong and may\nwant to be fixed, before we introduce another call site of pread().\n\nThanks.\n"},{"id":"238630","messageId":"1397154681-31803-1-git-send-email-yiannis.marangos@gmail.com","threadId":"36385","inReplyTo":"1397081197-14803-1-git-send-email-yiannis.marangos@gmail.com","subject":"[PATCH v7 1/2] Add xpread() and xpwrite()","fromName":"Yiannis Marangos","fromEmail":"yiannis.marangos@gmail.com","sentAt":"2014-04-10T18:31:20Z","receivedAt":"2014-04-10T18:31:20Z","isPatch":true,"sender":{"key":"yiannis.marangos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/194040?v=4"},"body":"xpread() and xpwrite() pay attention to EAGAIN/EINTR, so they will resume\nautomatically on interrupted call.\n\nSigned-off-by: Yiannis Marangos \n---\n builtin/index-pack.c | 2 +-\n compat/mmap.c | 4 +---\n git-compat-util.h | 2 ++\n wrapper.c | 36 ++++++++++++++++++++++++++++++++++++\n 4 files changed, 40 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex b9f6e12..1bac0f5 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -542,7 +542,7 @@ static void *unpack_data(struct object_entry *obj,\n \n \tdo {\n \t\tssize_t n = (len < 64*1024) ? len : 64*1024;\n-\t\tn = pread(pack_fd, inbuf, n, from);\n+\t\tn = xpread(pack_fd, inbuf, n, from);\n \t\tif (n < 0)\n \t\t\tdie_errno(_(\"cannot pread pack file\"));\n \t\tif (!n)\ndiff --git a/compat/mmap.c b/compat/mmap.c\nindex c9d46d1..7f662fe 100644\n--- a/compat/mmap.c\n+++ b/compat/mmap.c\n@@ -14,7 +14,7 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n \t}\n \n \twhile (n < length) {\n-\t\tssize_t count = pread(fd, (char *)start + n, length - n, offset + n);\n+\t\tssize_t count = xpread(fd, (char *)start + n, length - n, offset + n);\n \n \t\tif (count == 0) {\n \t\t\tmemset((char *)start+n, 0, length-n);\n@@ -22,8 +22,6 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n \t\t}\n \n \t\tif (count < 0) {\n-\t\t\tif (errno == EAGAIN || errno == EINTR)\n-\t\t\t\tcontinue;\n \t\t\tfree(start);\n \t\t\terrno = EACCES;\n \t\t\treturn MAP_FAILED;\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex f6d3a46..4da04c6 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -531,6 +531,8 @@ extern void *xcalloc(size_t nmemb, size_t size);\n extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n extern ssize_t xread(int fd, void *buf, size_t len);\n extern ssize_t xwrite(int fd, const void *buf, size_t len);\n+extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);\n+extern ssize_t xpwrite(int fd, const void *buf, size_t len, off_t offset);\n extern int xdup(int fd);\n extern FILE *xfdopen(int fd, const char *mode);\n extern int xmkstemp(char *template);\ndiff --git a/wrapper.c b/wrapper.c\nindex 0cc5636..25b7419 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -174,6 +174,42 @@ ssize_t xwrite(int fd, const void *buf, size_t len)\n \t}\n }\n \n+/*\n+ * xpread() is the same as pread(), but it automatically restarts pread()\n+ * operations with a recoverable error (EAGAIN and EINTR). xpread() DOES\n+ * NOT GUARANTEE that \"len\" bytes is read even if the data is available.\n+ */\n+ssize_t xpread(int fd, void *buf, size_t len, off_t offset)\n+{\n+\tssize_t nr;\n+\tif (len > MAX_IO_SIZE)\n+\t len = MAX_IO_SIZE;\n+\twhile (1) {\n+\t\tnr = pread(fd, buf, len, offset);\n+\t\tif ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n+\t\t\tcontinue;\n+\t\treturn nr;\n+\t}\n+}\n+\n+/*\n+ * xpwrite() is the same as pwrite(), but it automatically restarts pwrite()\n+ * operations with a recoverable error (EAGAIN and EINTR). xpwrite() DOES NOT\n+ * GUARANTEE that \"len\" bytes is written even if the operation is successful.\n+ */\n+ssize_t xpwrite(int fd, const void *buf, size_t len, off_t offset)\n+{\n+\tssize_t nr;\n+\tif (len > MAX_IO_SIZE)\n+\t len = MAX_IO_SIZE;\n+\twhile (1) {\n+\t\tnr = pwrite(fd, buf, len, offset);\n+\t\tif ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n+\t\t\tcontinue;\n+\t\treturn nr;\n+\t}\n+}\n+\n ssize_t read_in_full(int fd, void *buf, size_t count)\n {\n \tchar *p = buf;\n-- \n1.9.1\n"},{"id":"238631","messageId":"1397154681-31803-2-git-send-email-yiannis.marangos@gmail.com","threadId":"36385","inReplyTo":"1397154681-31803-1-git-send-email-yiannis.marangos@gmail.com","subject":"[PATCH v7 2/2] Verify index file before we opportunistically update it","fromName":"Yiannis Marangos","fromEmail":"yiannis.marangos@gmail.com","sentAt":"2014-04-10T18:31:21Z","receivedAt":"2014-04-10T18:31:21Z","isPatch":true,"sender":{"key":"yiannis.marangos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/194040?v=4"},"body":"Before we proceed to \"opportunistic update\" we must verify that the\ncurrent index file is the same as the one that we read before. There\nis a possible race if we don't do this. In the example below git-status\ndoes \"opportunistic update\" and git-rebase updates the index, but the\nrace can happen in general.\n\n 1. process A calls git-rebase (or does anything that uses the index)\n\n 2. process A applies 1st commit\n\n 3. process B calls git-status (or does anything that updates the index)\n\n 4. process B reads index\n\n 5. process A applies 2nd commit\n\n 6. process B takes the lock, then overwrites process A's changes.\n\n 7. process A applies 3rd commit\n\nAs an end result the 3rd commit will have a revert of the 2nd commit.\nWhen process B takes the lock, it needs to make sure that the index\nhasn't changed since step 4.\n\nSigned-off-by: Yiannis Marangos \n---\n\nVersion 4 contains fixes based on Junio's comments.\nVersion 5 fixs a typo in commit message (git-show -> git-status).\nVersion 6 removes verify_hdr() and use pread() instead of mmap().\nVersion 7 use xpread() (added with [PATCH v7 1/2]) instead of pread().\n\n cache.h | 1 +\n read-cache.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 47 insertions(+), 1 deletion(-)\n\ndiff --git a/cache.h b/cache.h\nindex 107ac61..0460f06 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -279,6 +279,7 @@ struct index_state {\n \t\t initialized : 1;\n \tstruct hashmap name_hash;\n \tstruct hashmap dir_hash;\n+\tunsigned char sha1[20];\n };\n \n extern struct index_state the_index;\ndiff --git a/read-cache.c b/read-cache.c\nindex ba13353..28de1a6 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1477,6 +1477,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \tif (verify_hdr(hdr, mmap_size) < 0)\n \t\tgoto unmap;\n \n+\thashcpy(istate->sha1, (unsigned char *)hdr + mmap_size - 20);\n \tistate->version = ntohl(hdr->hdr_version);\n \tistate->cache_nr = ntohl(hdr->hdr_entries);\n \tistate->cache_alloc = alloc_nr(istate->cache_nr);\n@@ -1760,6 +1761,50 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,\n \treturn result;\n }\n \n+/*\n+ * This function verifies if index_state has the correct sha1 of an index file.\n+ * Don't die if we have any other failure, just return 0.\n+ */\n+static int verify_index_from(const struct index_state *istate, const char *path)\n+{\n+\tint fd;\n+\tssize_t n;\n+\tstruct stat st;\n+\tunsigned char sha1[20];\n+\n+\tif (!istate->initialized)\n+\t\treturn 0;\n+\n+\tfd = open(path, O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn 0;\n+\n+\tif (fstat(fd, &st))\n+\t\tgoto out;\n+\n+\tif (st.st_size < sizeof(struct cache_header) + 20)\n+\t\tgoto out;\n+\n+\tn = xpread(fd, sha1, 20, st.st_size - 20);\n+\tif (n != 20)\n+\t\tgoto out;\n+\n+\tif (hashcmp(istate->sha1, sha1))\n+\t\tgoto out;\n+\n+\tclose(fd);\n+\treturn 1;\n+\n+out:\n+\tclose(fd);\n+\treturn 0;\n+}\n+\n+static int verify_index(const struct index_state *istate)\n+{\n+\treturn verify_index_from(istate, get_index_file());\n+}\n+\n static int has_racy_timestamp(struct index_state *istate)\n {\n \tint entries = istate->cache_nr;\n@@ -1779,7 +1824,7 @@ static int has_racy_timestamp(struct index_state *istate)\n void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)\n {\n \tif ((istate->cache_changed || has_racy_timestamp(istate)) &&\n-\t !write_index(istate, lockfile->fd))\n+\t verify_index(istate) && !write_index(istate, lockfile->fd))\n \t\tcommit_locked_index(lockfile);\n \telse\n \t\trollback_lock_file(lockfile);\n-- \n1.9.1\n"},{"id":"238632","messageId":"xmqq7g6xxcap.fsf@gitster.dls.corp.google.com","threadId":"36385","inReplyTo":"1397154681-31803-1-git-send-email-yiannis.marangos@gmail.com","subject":"Re: [PATCH v7 1/2] Add xpread() and xpwrite()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-10T18:35:42Z","receivedAt":"2014-04-10T18:35:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yiannis Marangos writes:\n\n> xpread() and xpwrite() pay attention to EAGAIN/EINTR, so they will resume\n> automatically on interrupted call.\n\nWe do not even use pwrite(); please don't add anything unnecessary\nand unexercised, like xpwrite(), as potential bugs in it will go\nunnoticed long after its introduction until it first gets used.\n\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index b9f6e12..1bac0f5 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -542,7 +542,7 @@ static void *unpack_data(struct object_entry *obj,\n> \n> \tdo {\n> \t\tssize_t n = (len < 64*1024) ? len : 64*1024;\n> -\t\tn = pread(pack_fd, inbuf, n, from);\n> +\t\tn = xpread(pack_fd, inbuf, n, from);\n> \t\tif (n < 0)\n> \t\t\tdie_errno(_(\"cannot pread pack file\"));\n> \t\tif (!n)\n\nOK.\n\n> diff --git a/compat/mmap.c b/compat/mmap.c\n> index c9d46d1..7f662fe 100644\n> --- a/compat/mmap.c\n> +++ b/compat/mmap.c\n> @@ -14,7 +14,7 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n> \t}\n> \n> \twhile (n < length) {\n> -\t\tssize_t count = pread(fd, (char *)start + n, length - n, offset + n);\n> +\t\tssize_t count = xpread(fd, (char *)start + n, length - n, offset + n);\n> \n> \t\tif (count == 0) {\n> \t\t\tmemset((char *)start+n, 0, length-n);\n> @@ -22,8 +22,6 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n> \t\t}\n> \n> \t\tif (count < 0) {\n> -\t\t\tif (errno == EAGAIN || errno == EINTR)\n> -\t\t\t\tcontinue;\n> \t\t\tfree(start);\n> \t\t\terrno = EACCES;\n> \t\t\treturn MAP_FAILED;\n\nOK.\n\n> diff --git a/wrapper.c b/wrapper.c\n> index 0cc5636..25b7419 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -174,6 +174,42 @@ ssize_t xwrite(int fd, const void *buf, size_t len)\n> \t}\n> }\n> \n> +/*\n> + * xpread() is the same as pread(), but it automatically restarts pread()\n> + * operations with a recoverable error (EAGAIN and EINTR). xpread() DOES\n> + * NOT GUARANTEE that \"len\" bytes is read even if the data is available.\n> + */\n> +ssize_t xpread(int fd, void *buf, size_t len, off_t offset)\n> +{\n> +\tssize_t nr;\n> +\tif (len > MAX_IO_SIZE)\n> +\t len = MAX_IO_SIZE;\n> +\twhile (1) {\n> +\t\tnr = pread(fd, buf, len, offset);\n> +\t\tif ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n> +\t\t\tcontinue;\n> +\t\treturn nr;\n> +\t}\n> +}\n\nOK.\n\nThanks.\n"},{"id":"238635","messageId":"20140410184404.GA11264@abyss.hitronhub.home","threadId":"36385","inReplyTo":"xmqq7g6xxcap.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v7 1/2] Add xpread() and xpwrite()","fromName":"Yiannis Marangos","fromEmail":"yiannis.marangos@gmail.com","sentAt":"2014-04-10T18:44:04Z","receivedAt":"2014-04-10T18:44:04Z","isPatch":true,"sender":{"key":"yiannis.marangos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/194040?v=4"},"body":"On Thu, Apr 10, 2014 at 11:35:42AM -0700, Junio C Hamano wrote:\n> We do not even use pwrite(); please don't add anything unnecessary\n> and unexercised, like xpwrite(), as potential bugs in it will go\n> unnoticed long after its introduction until it first gets used.\n\nCorrect, my mistake.\n"},{"id":"238638","messageId":"1397156052-1247-1-git-send-email-yiannis.marangos@gmail.com","threadId":"36385","inReplyTo":"1397081197-14803-1-git-send-email-yiannis.marangos@gmail.com","subject":"[PATCH v8 1/2] Add xpread()","fromName":"Yiannis Marangos","fromEmail":"yiannis.marangos@gmail.com","sentAt":"2014-04-10T18:54:12Z","receivedAt":"2014-04-10T18:54:12Z","isPatch":true,"sender":{"key":"yiannis.marangos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/194040?v=4"},"body":"xpread() pay attention to EAGAIN/EINTR, so it will resume\nautomatically on interrupted call.\n\nSigned-off-by: Yiannis Marangos \n---\n\nThis is actually the 2nd version of this commit but before I emailed\nit as version 7 because it's a dependency of [PATCH v7 2/2]. Is this\nthe correct way to introduce a new descendant commit? Sorry, It's my\nfirst time that I use mailing lists for contribution.\n\nVersion 8 removes xpwrite()\n\n builtin/index-pack.c | 2 +-\n compat/mmap.c | 4 +---\n git-compat-util.h | 1 +\n wrapper.c | 18 ++++++++++++++++++\n 4 files changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex b9f6e12..1bac0f5 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -542,7 +542,7 @@ static void *unpack_data(struct object_entry *obj,\n \n \tdo {\n \t\tssize_t n = (len < 64*1024) ? len : 64*1024;\n-\t\tn = pread(pack_fd, inbuf, n, from);\n+\t\tn = xpread(pack_fd, inbuf, n, from);\n \t\tif (n < 0)\n \t\t\tdie_errno(_(\"cannot pread pack file\"));\n \t\tif (!n)\ndiff --git a/compat/mmap.c b/compat/mmap.c\nindex c9d46d1..7f662fe 100644\n--- a/compat/mmap.c\n+++ b/compat/mmap.c\n@@ -14,7 +14,7 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n \t}\n \n \twhile (n < length) {\n-\t\tssize_t count = pread(fd, (char *)start + n, length - n, offset + n);\n+\t\tssize_t count = xpread(fd, (char *)start + n, length - n, offset + n);\n \n \t\tif (count == 0) {\n \t\t\tmemset((char *)start+n, 0, length-n);\n@@ -22,8 +22,6 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n \t\t}\n \n \t\tif (count < 0) {\n-\t\t\tif (errno == EAGAIN || errno == EINTR)\n-\t\t\t\tcontinue;\n \t\t\tfree(start);\n \t\t\terrno = EACCES;\n \t\t\treturn MAP_FAILED;\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex f6d3a46..e6a4159 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -531,6 +531,7 @@ extern void *xcalloc(size_t nmemb, size_t size);\n extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n extern ssize_t xread(int fd, void *buf, size_t len);\n extern ssize_t xwrite(int fd, const void *buf, size_t len);\n+extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);\n extern int xdup(int fd);\n extern FILE *xfdopen(int fd, const char *mode);\n extern int xmkstemp(char *template);\ndiff --git a/wrapper.c b/wrapper.c\nindex 0cc5636..cf71817 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -174,6 +174,24 @@ ssize_t xwrite(int fd, const void *buf, size_t len)\n \t}\n }\n \n+/*\n+ * xpread() is the same as pread(), but it automatically restarts pread()\n+ * operations with a recoverable error (EAGAIN and EINTR). xpread() DOES\n+ * NOT GUARANTEE that \"len\" bytes is read even if the data is available.\n+ */\n+ssize_t xpread(int fd, void *buf, size_t len, off_t offset)\n+{\n+\tssize_t nr;\n+\tif (len > MAX_IO_SIZE)\n+\t len = MAX_IO_SIZE;\n+\twhile (1) {\n+\t\tnr = pread(fd, buf, len, offset);\n+\t\tif ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n+\t\t\tcontinue;\n+\t\treturn nr;\n+\t}\n+}\n+\n ssize_t read_in_full(int fd, void *buf, size_t count)\n {\n \tchar *p = buf;\n-- \n1.9.1\n"},{"id":"238640","messageId":"5346ED16.3050607@kdbg.org","threadId":"36385","inReplyTo":"1397156052-1247-1-git-send-email-yiannis.marangos@gmail.com","subject":"Re: [PATCH v8 1/2] Add xpread()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-04-10T19:12:22Z","receivedAt":"2014-04-10T19:12:22Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10.04.2014 20:54, schrieb Yiannis Marangos:\n> +ssize_t xpread(int fd, void *buf, size_t len, off_t offset)\n> +{\n> +\tssize_t nr;\n> +\tif (len > MAX_IO_SIZE)\n> +\t len = MAX_IO_SIZE;\n\nOdd indentation here.\n\n-- Hannes\n"},{"id":"238652","messageId":"xmqqtxa1vvms.fsf@gitster.dls.corp.google.com","threadId":"36385","inReplyTo":"5346ED16.3050607@kdbg.org","subject":"Re: [PATCH v8 1/2] Add xpread()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-10T19:20:59Z","receivedAt":"2014-04-10T19:20:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 10.04.2014 20:54, schrieb Yiannis Marangos:\n>> +ssize_t xpread(int fd, void *buf, size_t len, off_t offset)\n>> +{\n>> +\tssize_t nr;\n>> +\tif (len > MAX_IO_SIZE)\n>> +\t len = MAX_IO_SIZE;\n>\n> Odd indentation here.\n>\n> -- Hannes\n\nGood eyes, even though this is copy&pasting an existing error from\nsurrounding code ;-)\n\nI'll queue this instead (rebased on top of maint-1.8.5 even though I\ndoubt we would be issuing 1.8.5.6).\n\n-- >8 --\nFrom: Yiannis Marangos \nDate: Thu, 10 Apr 2014 21:54:12 +0300\nSubject: [PATCH] wrapper.c: add xpread() similar to xread()\n\nIt is a common mistake to call read(2)/pread(2) and forget to\nanticipate that they may return error with EAGAIN/EINTR when the\nsystem call is interrupted.\n\nWe have xread() helper to relieve callers of read(2) from having to\nworry about it; add xpread() helper to do the same for pread(2).\n\nUpdate the caller in the builtin/index-pack.c and the mmap emulation\nin compat/.\n\nSigned-off-by: Yiannis Marangos \nSigned-off-by: Junio C Hamano \n---\n builtin/index-pack.c | 2 +-\n compat/mmap.c | 4 +---\n git-compat-util.h | 1 +\n wrapper.c | 18 ++++++++++++++++++\n 4 files changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 9e9eb4b..e7a6b53 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -542,7 +542,7 @@ static void *unpack_data(struct object_entry *obj,\n \n \tdo {\n \t\tssize_t n = (len < 64*1024) ? len : 64*1024;\n-\t\tn = pread(pack_fd, inbuf, n, from);\n+\t\tn = xpread(pack_fd, inbuf, n, from);\n \t\tif (n < 0)\n \t\t\tdie_errno(_(\"cannot pread pack file\"));\n \t\tif (!n)\ndiff --git a/compat/mmap.c b/compat/mmap.c\nindex c9d46d1..7f662fe 100644\n--- a/compat/mmap.c\n+++ b/compat/mmap.c\n@@ -14,7 +14,7 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n \t}\n \n \twhile (n < length) {\n-\t\tssize_t count = pread(fd, (char *)start + n, length - n, offset + n);\n+\t\tssize_t count = xpread(fd, (char *)start + n, length - n, offset + n);\n \n \t\tif (count == 0) {\n \t\t\tmemset((char *)start+n, 0, length-n);\n@@ -22,8 +22,6 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n \t\t}\n \n \t\tif (count < 0) {\n-\t\t\tif (errno == EAGAIN || errno == EINTR)\n-\t\t\t\tcontinue;\n \t\t\tfree(start);\n \t\t\terrno = EACCES;\n \t\t\treturn MAP_FAILED;\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 7776f12..9eec5fb 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -534,6 +534,7 @@ extern void *xcalloc(size_t nmemb, size_t size);\n extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n extern ssize_t xread(int fd, void *buf, size_t len);\n extern ssize_t xwrite(int fd, const void *buf, size_t len);\n+extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);\n extern int xdup(int fd);\n extern FILE *xfdopen(int fd, const char *mode);\n extern int xmkstemp(char *template);\ndiff --git a/wrapper.c b/wrapper.c\nindex 0cc5636..5b3c7fc 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -174,6 +174,24 @@ ssize_t xwrite(int fd, const void *buf, size_t len)\n \t}\n }\n \n+/*\n+ * xpread() is the same as pread(), but it automatically restarts pread()\n+ * operations with a recoverable error (EAGAIN and EINTR). xpread() DOES\n+ * NOT GUARANTEE that \"len\" bytes is read even if the data is available.\n+ */\n+ssize_t xpread(int fd, void *buf, size_t len, off_t offset)\n+{\n+\tssize_t nr;\n+\tif (len > MAX_IO_SIZE)\n+\t\tlen = MAX_IO_SIZE;\n+\twhile (1) {\n+\t\tnr = pread(fd, buf, len, offset);\n+\t\tif ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n+\t\t\tcontinue;\n+\t\treturn nr;\n+\t}\n+}\n+\n ssize_t read_in_full(int fd, void *buf, size_t count)\n {\n \tchar *p = buf;\n-- \n1.9.2-590-g468068b\n"},{"id":"238654","messageId":"xmqqppkpvv9m.fsf@gitster.dls.corp.google.com","threadId":"36385","inReplyTo":"1397154681-31803-2-git-send-email-yiannis.marangos@gmail.com","subject":"Re: [PATCH v7 2/2] Verify index file before we opportunistically update it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-10T19:28:53Z","receivedAt":"2014-04-10T19:28:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yiannis Marangos writes:\n\n> +\tn = xpread(fd, sha1, 20, st.st_size - 20);\n> +\tif (n != 20)\n> +\t\tgoto out;\n\nI think it is possible for pread(2) to give you a short-read.\n\nThe existing callers of emulated mmap and index-pack are prepared to\nhandle a short-read correctly, but I do not think this code does.\n\nI'll queue this instead in the meantime.\n\n-- >8 --\nFrom: Yiannis Marangos \nDate: Thu, 10 Apr 2014 21:31:21 +0300\nSubject: [PATCH] read-cache.c: verify index file before we opportunistically update it\n\nBefore we proceed to opportunistically update the index (often done\nby an otherwise read-only operation like \"git status\" and \"git diff\"\nthat internally refreshes the index), we must verify that the\ncurrent index file is the same as the one that we read earlier\nbefore we took the lock on it, in order to avoid a possible race.\n\nIn the example below git-status does \"opportunistic update\" and\ngit-rebase updates the index, but the race can happen in general.\n\n 1. process A calls git-rebase (or does anything that uses the index)\n\n 2. process A applies 1st commit\n\n 3. process B calls git-status (or does anything that updates the index)\n\n 4. process B reads index\n\n 5. process A applies 2nd commit\n\n 6. process B takes the lock, then overwrites process A's changes.\n\n 7. process A applies 3rd commit\n\nAs an end result the 3rd commit will have a revert of the 2nd commit.\nWhen process B takes the lock, it needs to make sure that the index\nhasn't changed since step 4.\n\nSigned-off-by: Yiannis Marangos \nSigned-off-by: Junio C Hamano \n---\n cache.h | 3 +++\n read-cache.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-\n wrapper.c | 20 ++++++++++++++++++++\n 3 files changed, 69 insertions(+), 1 deletion(-)\n\ndiff --git a/cache.h b/cache.h\nindex ce377e1..9244c38 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -279,6 +279,7 @@ struct index_state {\n \t\t initialized : 1;\n \tstruct hash_table name_hash;\n \tstruct hash_table dir_hash;\n+\tunsigned char sha1[20];\n };\n \n extern struct index_state the_index;\n@@ -1199,6 +1200,8 @@ extern void fsync_or_die(int fd, const char *);\n \n extern ssize_t read_in_full(int fd, void *buf, size_t count);\n extern ssize_t write_in_full(int fd, const void *buf, size_t count);\n+extern ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset);\n+\n static inline ssize_t write_str_in_full(int fd, const char *str)\n {\n \treturn write_in_full(fd, str, strlen(str));\ndiff --git a/read-cache.c b/read-cache.c\nindex 33dd676..f4a0d61 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1464,6 +1464,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \tif (verify_hdr(hdr, mmap_size) < 0)\n \t\tgoto unmap;\n \n+\thashcpy(istate->sha1, (unsigned char *)hdr + mmap_size - 20);\n \tistate->version = ntohl(hdr->hdr_version);\n \tistate->cache_nr = ntohl(hdr->hdr_entries);\n \tistate->cache_alloc = alloc_nr(istate->cache_nr);\n@@ -1747,6 +1748,50 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,\n \treturn result;\n }\n \n+/*\n+ * This function verifies if index_state has the correct sha1 of the\n+ * index file. Don't die if we have any other failure, just return 0.\n+ */\n+static int verify_index_from(const struct index_state *istate, const char *path)\n+{\n+\tint fd;\n+\tssize_t n;\n+\tstruct stat st;\n+\tunsigned char sha1[20];\n+\n+\tif (!istate->initialized)\n+\t\treturn 0;\n+\n+\tfd = open(path, O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn 0;\n+\n+\tif (fstat(fd, &st))\n+\t\tgoto out;\n+\n+\tif (st.st_size < sizeof(struct cache_header) + 20)\n+\t\tgoto out;\n+\n+\tn = pread_in_full(fd, sha1, 20, st.st_size - 20);\n+\tif (n != 20)\n+\t\tgoto out;\n+\n+\tif (hashcmp(istate->sha1, sha1))\n+\t\tgoto out;\n+\n+\tclose(fd);\n+\treturn 1;\n+\n+out:\n+\tclose(fd);\n+\treturn 0;\n+}\n+\n+static int verify_index(const struct index_state *istate)\n+{\n+\treturn verify_index_from(istate, get_index_file());\n+}\n+\n static int has_racy_timestamp(struct index_state *istate)\n {\n \tint entries = istate->cache_nr;\n@@ -1766,7 +1811,7 @@ static int has_racy_timestamp(struct index_state *istate)\n void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)\n {\n \tif ((istate->cache_changed || has_racy_timestamp(istate)) &&\n-\t !write_index(istate, lockfile->fd))\n+\t verify_index(istate) && !write_index(istate, lockfile->fd))\n \t\tcommit_locked_index(lockfile);\n \telse\n \t\trollback_lock_file(lockfile);\ndiff --git a/wrapper.c b/wrapper.c\nindex 5b3c7fc..bc1bfb8 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -232,6 +232,26 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)\n \treturn total;\n }\n \n+ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset)\n+{\n+\tchar *p = buf;\n+\tssize_t total = 0;\n+\n+\twhile (count > 0) {\n+\t\tssize_t loaded = xpread(fd, p, count, offset);\n+\t\tif (loaded < 0)\n+\t\t\treturn -1;\n+\t\tif (loaded == 0)\n+\t\t\treturn total;\n+\t\tcount -= loaded;\n+\t\tp += loaded;\n+\t\ttotal += loaded;\n+\t\toffset += loaded;\n+\t}\n+\n+\treturn total;\n+}\n+\n int xdup(int fd)\n {\n \tint ret = dup(fd);\n-- \n1.9.2-590-g468068b\n"},{"id":"238668","messageId":"CACsJy8A5VJM4gnJGazkzniv5YxRx0xR7YEFpQbU+kVGofQ1b_g@mail.gmail.com","threadId":"36385","inReplyTo":"xmqqppkpvv9m.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v7 2/2] Verify index file before we opportunistically update it","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-04-11T02:57:18Z","receivedAt":"2014-04-11T02:57:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Apr 11, 2014 at 2:28 AM, Junio C Hamano wrote:\n> Yiannis Marangos writes:\n>\n>> + n = xpread(fd, sha1, 20, st.st_size - 20);\n>> + if (n != 20)\n>> + goto out;\n>\n> I think it is possible for pread(2) to give you a short-read.\n>\n> The existing callers of emulated mmap and index-pack are prepared to\n> handle a short-read correctly, but I do not think this code does.\n>\n> I'll queue this instead in the meantime.\n\nThere are two things to sort out (sorry I can't spend much time on it\nright now): should the same sha1 test be done in write_index(), in\naddition to update_index_if_able(). And what do we do about\nistate->sha1[] in discard_index(), keep it or clear it.\n-- \nDuy\n"},{"id":"238670","messageId":"53479DFD.4020702@web.de","threadId":"36385","inReplyTo":"xmqqppkpvv9m.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v7 2/2] Verify index file before we opportunistically update it","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-04-11T07:47:09Z","receivedAt":"2014-04-11T07:47:09Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-04-10 21.28, Junio C Hamano wrote:\n> Yiannis Marangos writes:\n> \n>> +\tn = xpread(fd, sha1, 20, st.st_size - 20);\n>> +\tif (n != 20)\n>> +\t\tgoto out;\n> \n> I think it is possible for pread(2) to give you a short-read.\n> \n> The existing callers of emulated mmap and index-pack are prepared to\n> handle a short-read correctly, but I do not think this code does.\n> \n> I'll queue this instead in the meantime.\n> \n> -- >8 --\n> From: Yiannis Marangos \n> Date: Thu, 10 Apr 2014 21:31:21 +0300\n> Subject: [PATCH] read-cache.c: verify index file before we opportunistically update it\n> \n> Before we proceed to opportunistically update the index (often done\n> by an otherwise read-only operation like \"git status\" and \"git diff\"\n> that internally refreshes the index), we must verify that the\n> current index file is the same as the one that we read earlier\n> before we took the lock on it, in order to avoid a possible race.\n> \n> In the example below git-status does \"opportunistic update\" and\n> git-rebase updates the index, but the race can happen in general.\nI'm not sure if we need the second or third commit of process A at all.\nMy understanding is that the simplified version will have problems as well:\n\n 1. process A calls git-rebase (or does anything that updates the index)\n 2. process change\n 3. process B calls git-status (or does anything that updates the index)\n 4. process B reads the index file into memory\n 5. process change\n 6. process A applies a commit:\n - read the index into memory\n - take the lock\n - update the index file on disc\n - release the lock\n 7. process change\n 8. process B applies a commit:\n - take the lock\n - update the index in memory and write the index file to disc\n - release the lock\n\n Now process B has overwritten the commit from process A, which is wrong.\n\nThe new code works like this:\n\n 8. process B applies a commit:\n - take the lock\n - verifies tha the index file on disc has the same sha as the one read before\n # And if not: What do we do? die() or retry() ?\n - update the index file on disc\n - release the lock\n[]\n> \n> +static int verify_index_from(const struct index_state *istate, const char *path)\n> +{\n> +\tint fd;\n> +\tssize_t n;\n> +\tstruct stat st;\n> +\tunsigned char sha1[20];\n> +\n> +\tif (!istate->initialized)\n> +\t\treturn 0;\n> +\n> +\tfd = open(path, O_RDONLY);\n> +\tif (fd < 0)\n> +\t\treturn 0;\n> +\n> +\tif (fstat(fd, &st))\n> +\t\tgoto out;\n> +\n> +\tif (st.st_size < sizeof(struct cache_header) + 20)\n> +\t\tgoto out;\n> +\n> +\tn = pread_in_full(fd, sha1, 20, st.st_size - 20);\nMinor :\nWhat is the advantage of pread() against a lseek()/read_in_full() combo?\nfd is opened and used only in one thread.\nIntroducing pread()/ pread_in_full() could be done in an other commit,\nor do I miss something ?\n> +\tif (n != 20)\n> +\t\tgoto out;\n\n> +static int verify_index(const struct index_state *istate)\n> +{\n> +\treturn verify_index_from(istate, get_index_file());\n> +}\n> +\nMinor:\nDo we really need the wrapper function verify_index_from()?\nIt seems as if the whole code from verify_index_from() could go into\nverify_index(), which will call get_index_file()\n \n \n> static int has_racy_timestamp(struct index_state *istate)\n> {\n> \tint entries = istate->cache_nr;\n> @@ -1766,7 +1811,7 @@ static int has_racy_timestamp(struct index_state *istate)\n> void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)\n> {\n> \tif ((istate->cache_changed || has_racy_timestamp(istate)) &&\n> -\t !write_index(istate, lockfile->fd))\n> +\t verify_index(istate) && !write_index(istate, lockfile->fd))\n> \t\tcommit_locked_index(lockfile);\n> \telse\n> \t\trollback_lock_file(lockfile);\n> diff --git a/wrapper.c b/wrapper.c\n> index 5b3c7fc..bc1bfb8 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -232,6 +232,26 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)\n> \treturn total;\n> }\n> \n> +ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset)\n> +{\n> +\tchar *p = buf;\n> +\tssize_t total = 0;\n> +\n> +\twhile (count > 0) {\n> +\t\tssize_t loaded = xpread(fd, p, count, offset);\n> +\t\tif (loaded < 0)\n> +\t\t\treturn -1;\n> +\t\tif (loaded == 0)\n> +\t\t\treturn total;\n> +\t\tcount -= loaded;\n> +\t\tp += loaded;\n> +\t\ttotal += loaded;\n> +\t\toffset += loaded;\n> +\t}\n> +\n> +\treturn total;\n> +}\n> +\n> int xdup(int fd)\n> {\n> \tint ret = dup(fd);\n> \n"},{"id":"238678","messageId":"CACsJy8DGq8Q8i8pgYivcMhnk99gYhnKvSWUsetFL=+3L8YLPew@mail.gmail.com","threadId":"36385","inReplyTo":"xmqqppkpvv9m.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v7 2/2] Verify index file before we opportunistically update it","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-04-11T10:36:58Z","receivedAt":"2014-04-11T10:36:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Apr 11, 2014 at 2:28 AM, Junio C Hamano wrote:\n> +static int verify_index_from(const struct index_state *istate, const char *path)\n> +{\n> + int fd;\n> + ssize_t n;\n> + struct stat st;\n> + unsigned char sha1[20];\n> +\n> + if (!istate->initialized)\n> + return 0;\n> +\n> + fd = open(path, O_RDONLY);\n> + if (fd < 0)\n> + return 0;\n\nSuppose another process \"git rm --cached :/\" is racing with us and\nthis imaginary git is so smart that it figures if nothing valueable is\nleft in the index, there's no need to write the index down at all. So\nit removes $GIT_DIR/index, then $GIT_DIR/index.lock. When we come\nhere, we see ENOENT, but we should return 1 instead because the file\nremoval in this case is a form of change.\n\nThat opens a question about writing a new index. I think we could use\nall-zero sha-1 as the indicator that this is a fresh new index. If\nistate->sha1[] is all-zero and no index file exists, then we do not\nneed to verify (i.e. return 0). However if istate->sha1[] is all-zero,\nbut $GIT_DIR/index exists, then return 1.\n\nI'm still not sure if elsewhere in the code base we read\n$GIT_DIR/index to active_index, create a new index_state, copy entries\nover (but not active_index.sha1[]), then write the _new_ index_state\ndown. That would hit the \"however\" statement above and incorrectly\nreturn 1.\n\nI suppose that all other errors except ENOENT could be safely ignored\n(i.e. return 0 regardless of istate->sha1[]).\n\n> +\n> + if (fstat(fd, &st))\n> + goto out;\n> +\n> + if (st.st_size < sizeof(struct cache_header) + 20)\n> + goto out;\n> +\n> + n = pread_in_full(fd, sha1, 20, st.st_size - 20);\n> + if (n != 20)\n> + goto out;\n> +\n> + if (hashcmp(istate->sha1, sha1))\n> + goto out;\n> +\n> + close(fd);\n> + return 1;\n> +\n> +out:\n> + close(fd);\n> + return 0;\n> +}\n> +\n> +static int verify_index(const struct index_state *istate)\n> +{\n> + return verify_index_from(istate, get_index_file());\n> +}\n> +\n> static int has_racy_timestamp(struct index_state *istate)\n> {\n> int entries = istate->cache_nr;\n> @@ -1766,7 +1811,7 @@ static int has_racy_timestamp(struct index_state *istate)\n> void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)\n> {\n> if ((istate->cache_changed || has_racy_timestamp(istate)) &&\n> - !write_index(istate, lockfile->fd))\n> + verify_index(istate) && !write_index(istate, lockfile->fd))\n> commit_locked_index(lockfile);\n> else\n> rollback_lock_file(lockfile);\n> diff --git a/wrapper.c b/wrapper.c\n> index 5b3c7fc..bc1bfb8 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -232,6 +232,26 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)\n> return total;\n> }\n>\n> +ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset)\n> +{\n> + char *p = buf;\n> + ssize_t total = 0;\n> +\n> + while (count > 0) {\n> + ssize_t loaded = xpread(fd, p, count, offset);\n> + if (loaded < 0)\n> + return -1;\n> + if (loaded == 0)\n> + return total;\n> + count -= loaded;\n> + p += loaded;\n> + total += loaded;\n> + offset += loaded;\n> + }\n> +\n> + return total;\n> +}\n> +\n> int xdup(int fd)\n> {\n> int ret = dup(fd);\n> --\n> 1.9.2-590-g468068b\n>\n>\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n\n\n\n-- \nDuy\n"},{"id":"238706","messageId":"20140411155857.GA12742@abyss.hitronhub.home","threadId":"36385","inReplyTo":"53479DFD.4020702@web.de","subject":"Re: [PATCH v7 2/2] Verify index file before we opportunistically update it","fromName":"Yiannis Marangos","fromEmail":"yiannis.marangos@gmail.com","sentAt":"2014-04-11T15:58:57Z","receivedAt":"2014-04-11T15:58:57Z","isPatch":true,"sender":{"key":"yiannis.marangos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/194040?v=4"},"body":"On Fri, Apr 11, 2014 at 09:47:09AM +0200, Torsten Bögershausen wrote:\n> 6. process A applies a commit:\n> - read the index into memory\n> - take the lock\n> - update the index file on disc\n> - release the lock\n\nSo here we can have race condition. Maybe we should implement Duy's\nidea of verifying the sha1 in write_index()?\n\n\n> The new code works like this:\n>\n> 8. process B applies a commit:\n> - take the lock\n> - verifies tha the index file on disc has the same sha as the one read before\n> # And if not: What do we do? die() or retry() ?\n> - update the index file on disc\n> - release the lock\n\nIMO, we must never die() if we want to do opportunistic update and we\nshould never retry because it's a bit expensive if we do opportunistic update.\n\n\n--\nYiannis\n"},{"id":"238733","messageId":"xmqqtx9zsm8p.fsf@gitster.dls.corp.google.com","threadId":"36385","inReplyTo":"CACsJy8A5VJM4gnJGazkzniv5YxRx0xR7YEFpQbU+kVGofQ1b_g@mail.gmail.com","subject":"Re: [PATCH v7 2/2] Verify index file before we opportunistically update it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-11T19:24:22Z","receivedAt":"2014-04-11T19:24:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Fri, Apr 11, 2014 at 2:28 AM, Junio C Hamano wrote:\n>> Yiannis Marangos writes:\n>>\n>>> + n = xpread(fd, sha1, 20, st.st_size - 20);\n>>> + if (n != 20)\n>>> + goto out;\n>>\n>> I think it is possible for pread(2) to give you a short-read.\n>>\n>> The existing callers of emulated mmap and index-pack are prepared to\n>> handle a short-read correctly, but I do not think this code does.\n>>\n>> I'll queue this instead in the meantime.\n>\n> There are two things to sort out (sorry I can't spend much time on it\n> right now): should the same sha1 test be done in write_index(), in\n> addition to update_index_if_able(). And what do we do about\n> istate->sha1[] in discard_index(), keep it or clear it.\n\nYeah, I was hoping that the real write codepath (as opposed to \"this\nis read only and we read the index without holding a lock---now we\nnoticed that the index needs refreshing, and we know how the\nresulting refreshed index should look like, perhaps we can write it\nto save cycles for other processes\" codepath where we cannot and\nshould not take a lock early) would take the lock and then read, but\nbecause that is not the way they work, the need the same protection,\nI would think. As discard_index() is not \"we are removing all the\nentries because the user told us to 'rm -r .'\" but is \"for the\npurpose of our internal processing we do not need the old contents\nof the index\", my knee-jerk reaction is that we should not clear it.\n\nAny operation that makes the resulting index empty (i.e. no path\nbeing tracked) should still write an empty index (with the usual\nheader and the most importantly the trailing file checksum), but\nthat goes without saying.\n"},{"id":"238749","messageId":"xmqqmwfrr3zw.fsf@gitster.dls.corp.google.com","threadId":"36385","inReplyTo":"xmqqtx9zsm8p.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v7 2/2] Verify index file before we opportunistically update it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-11T20:43:47Z","receivedAt":"2014-04-11T20:43:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Yeah, I was hoping that the real write codepath (as opposed to \"this\n> is read only and we read the index without holding a lock---now we\n> noticed that the index needs refreshing, and we know how the\n> resulting refreshed index should look like, perhaps we can write it\n> to save cycles for other processes\" codepath where we cannot and\n> should not take a lock early) would take the lock and then read, but\n> because that is not the way they work, the need the same protection,\n> I would think.\n\nHaving said that, nobody sane would be running two simultaneous\noperations that are clearly write-oriented competing with each other\nagainst the same index file. So in that sense that can be done as a\nless urgent follow-up for this topic.\n"},{"id":"238757","messageId":"20140411233005.GA14564@abyss","threadId":"36385","inReplyTo":"xmqqmwfrr3zw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v7 2/2] Verify index file before we opportunistically update it","fromName":"Yiannis Marangos","fromEmail":"yiannis.marangos@gmail.com","sentAt":"2014-04-11T23:30:05Z","receivedAt":"2014-04-11T23:30:05Z","isPatch":true,"sender":{"key":"yiannis.marangos@gmail.com","avatar":"https://avatars.githubusercontent.com/u/194040?v=4"},"body":"On Fri, Apr 11, 2014 at 01:43:47PM -0700, Junio C Hamano wrote:\n> Having said that, nobody sane would be running two simultaneous\n> operations that are clearly write-oriented competing with each other\n> against the same index file. So in that sense that can be done as a\n> less urgent follow-up for this topic.\n\nI'm willing to take this task, I decide to spend some of my free time\nfor git development. But since I'm a newbie with it's architecture I\ndon't know when it will be ready because I want to explore the code a\nbit.\n\n--\nYiannis\n"},{"id":"238761","messageId":"CACsJy8ASAvopmyhawmUkuFk4DjBd2opL1kxu_jtopTDr80BWPg@mail.gmail.com","threadId":"36385","inReplyTo":"xmqqmwfrr3zw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v7 2/2] Verify index file before we opportunistically update it","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-04-12T00:10:18Z","receivedAt":"2014-04-12T00:10:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Apr 12, 2014 at 3:43 AM, Junio C Hamano wrote:\n> Having said that, nobody sane would be running two simultaneous\n> operations that are clearly write-oriented competing with each other\n> against the same index file.\n\nWhen it comes to racing, sanity does not matter much. People could\njust do it without thinking what exactly is happening behind the\nscene.\n\n> So in that sense that can be done as a less urgent follow-up for this topic.\n\nYeah if racing at refresh time is a real problem, sure we should solve it first.\n-- \nDuy\n"},{"id":"238762","messageId":"7vmwfrmb6a.fsf@alter.siamese.dyndns.org","threadId":"36385","inReplyTo":"CACsJy8ASAvopmyhawmUkuFk4DjBd2opL1kxu_jtopTDr80BWPg@mail.gmail.com","subject":"Re: [PATCH v7 2/2] Verify index file before we opportunistically update it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-12T04:19:57Z","receivedAt":"2014-04-12T04:19:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Sat, Apr 12, 2014 at 3:43 AM, Junio C Hamano wrote:\n>> Having said that, nobody sane would be running two simultaneous\n>> operations that are clearly write-oriented competing with each other\n>> against the same index file.\n>\n> When it comes to racing, sanity does not matter much. People could\n> just do it without thinking what exactly is happening behind the\n> scene.\n\nWell, \"a race is a race is a race\" would also be my knee-jerk\nreaction when anybody utters the word \"race\", but you need to\nrealize that we are not talking about races like object creation\nwhile \"gc --auto\" running in the background or two pushes trying to\nupdate the same ref to different values, which are meaningful use\ncases.\n\nWhat is the race under discussion about? It is about the index,\nwhich corresponds one-to-one to the working tree, so in order for\nthe \"race\" to matter, you need to be racing against another process\nthat is not cooperating with you (e.g. a continuous and uncontrolled\n\"git add\" updating the index while you are doing a real work),\nmucking with the index in the same working tree. How could such a\nworkflow any useful in the real life?\n\nIn the spectrum between useful and insane, there is a point where we\nshould just tell the insane: don't do it then.\n\nHaving said that...\n\n>> So in that sense that can be done as a less urgent follow-up for this topic.\n>\n> Yeah if racing at refresh time is a real problem, sure we should solve it first.\n\n... In order to solve the write-vs-write competition in a useful\nway, you must serialize the competing accesses. E.g. \"git add\"\nwould first take a write lock on the index before read_cache(), and\nthen do its operation and finally release the lock by the usual\nwrite-to-lock-close-then-rename dance.\n\nThe lazy \"read and refresh in-core first, hoping that we did not\ncompete with anybody, and then check just before writing out after\ntaking the lock\" is a very good solution for the opportunistic\nupdate codepath, because it is an option not to write the result out\nwhen there actually was an update by somebody else. But such an\nopportunistic locking scheme does not work for write-vs-write\ncompetition. Upon a real conflict, you need to fail the entire\noperation.\n"},{"id":"238765","messageId":"7v1tx3m3hy.fsf@alter.siamese.dyndns.org","threadId":"36385","inReplyTo":"7vmwfrmb6a.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v7 2/2] Verify index file before we opportunistically update it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-12T07:05:45Z","receivedAt":"2014-04-12T07:05:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> What is the race under discussion about? It is about the index,\n> which corresponds one-to-one to the working tree, so in order for\n> the \"race\" to matter, you need to be racing against another process\n> that is not cooperating with you (e.g. a continuous and uncontrolled\n> \"git add\" updating the index while you are doing a real work),\n> mucking with the index in the same working tree. How could such a\n> workflow any useful in the real life?\n>\n> In the spectrum between useful and insane, there is a point where we\n> should just tell the insane: don't do it then.\n>\n> Having said that...\n>\n>>> So in that sense that can be done as a less urgent follow-up for this topic.\n>>\n>> Yeah if racing at refresh time is a real problem, sure we should solve it first.\n>\n> ... In order to solve the write-vs-write competition in a useful\n> way, you must serialize the competing accesses. E.g. \"git add\"\n> would first take a write lock on the index before read_cache(), and\n> then do its operation and finally release the lock by the usual\n> write-to-lock-close-then-rename dance.\n\nExtending on this a bit.\n\nHere I didn't mean the traditional \".lock\" we create via the\nlockfile() API. Rather, we would want to use fcntl/flock style lock\nthat lets others _wait_, not _fail_. Because...\n\n> The lazy \"read and refresh in-core first, hoping that we did not\n> compete with anybody, and then check just before writing out after\n> taking the lock\" is a very good solution for the opportunistic\n> update codepath, because it is an option not to write the result out\n> when there actually was an update by somebody else. But such an\n> opportunistic locking scheme does not work for write-vs-write\n> competition. Upon a real conflict, you need to fail the entire\n> operation.\n\n...having multiple conflicting writers on the single index file is\nwhat you thought about worth protecting against. When somebody else\nis pounding on the index file you are trying to prepare your next\ncommmit in, with his writing that can unexpectedly overwrite what\nyou prepared, you would at least want the accesses serialized,\nwithout getting half of your attempt to \"git add\" fail (and having\nto redo). For that, you would want your \"git add\" to wait while the\nother party is mucking with the index under lock, instead of\nfailing, which is what you would get from the traditional lockfile\nAPI based locking.\n\nBut that still takes us back to the \"don't do it then\". It is true\nthat, with serialization, you may be able to guarantee that one \"git\nadd\" you do would start from one state (which may record the state\nof your previous \"git add\", or which may have further been modified\nby the other process) and ends with whatever that \"git add\" added,\nwithout any other change. But even in that case, when you finally\ndo a \"git commit\", can you say you know what is in the index? I do\nnot think so. After all, the root cause of the \"race\" issue is that\nthe other process pounds at the same index while you are working on\nit, without any coordination with you.\n\nSo...\n"},{"id":"238766","messageId":"CACsJy8C=a6a4COBdG9uLo5XNViK6Lb3+ZyWM1P_e9p7qu+shkQ@mail.gmail.com","threadId":"36385","inReplyTo":"7vmwfrmb6a.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v7 2/2] Verify index file before we opportunistically update it","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-04-12T10:13:48Z","receivedAt":"2014-04-12T10:13:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Apr 12, 2014 at 11:19 AM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n>\n>> On Sat, Apr 12, 2014 at 3:43 AM, Junio C Hamano wrote:\n>>> Having said that, nobody sane would be running two simultaneous\n>>> operations that are clearly write-oriented competing with each other\n>>> against the same index file.\n>>\n>> When it comes to racing, sanity does not matter much. People could\n>> just do it without thinking what exactly is happening behind the\n>> scene.\n>\n> Well, \"a race is a race is a race\" would also be my knee-jerk\n> reaction when anybody utters the word \"race\", but you need to\n> realize that we are not talking about races like object creation\n> while \"gc --auto\" running in the background or two pushes trying to\n> update the same ref to different values, which are meaningful use\n> cases.\n>\n> What is the race under discussion about? It is about the index,\n> which corresponds one-to-one to the working tree, so in order for\n> the \"race\" to matter, you need to be racing against another process\n> that is not cooperating with you (e.g. a continuous and uncontrolled\n> \"git add\" updating the index while you are doing a real work),\n> mucking with the index in the same working tree. How could such a\n> workflow any useful in the real life?\n>\n> In the spectrum between useful and insane, there is a point where we\n> should just tell the insane: don't do it then.\n\nThe thing is people do not usually have that level of knowledge of how\ngit works. They could write a cron job to do something in some repos,\nnot aware at all about these non-cooperations. Telling people not to\nautomatically touch a git directory at all is a bit extreme. I think\nthis patch is about guarding the user from shooting themselves. Either\na command works correctly, not not work at all. A process' dying is a\nway of telling people \"this is insane\" without having to draw a line\nbetween dos and dont's in documents.\n\nSerializing write access to make both competing processes is nice, but\nthat's a separate step that may or may not be worth pursuing. And I'm\ntowards not worth pursuing. As you metioned in the next mail,\nserializing helps two competing processes, but not two command\nsequences.\n-- \nDuy\n"},{"id":"238857","messageId":"xmqq7g6rpwxn.fsf@gitster.dls.corp.google.com","threadId":"36385","inReplyTo":"CACsJy8C=a6a4COBdG9uLo5XNViK6Lb3+ZyWM1P_e9p7qu+shkQ@mail.gmail.com","subject":"Re: [PATCH v7 2/2] Verify index file before we opportunistically update it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-14T18:50:44Z","receivedAt":"2014-04-14T18:50:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Sat, Apr 12, 2014 at 11:19 AM, Junio C Hamano wrote:\n>\n>> In the spectrum between useful and insane, there is a point where we\n>> should just tell the insane: don't do it then.\n>\n> ... A process' dying is a\n> way of telling people \"this is insane\" without having to draw a line\n> between dos and dont's in documents.\n\nFair enough. Instead of \"that is insane and you may end up\nrecording what you do not intend to\", being able to say \"that is\ninsane and you will see some commands failing when you may end up\nbeing in such a situation\" is a lot more explicit.\n\nThanks for injecting some sanity to me.\n\n> Serializing write access to make both competing processes is nice, but\n> that's a separate step that may or may not be worth pursuing. And I'm\n> towards not worth pursuing.\n\nI think we are in agreement on that point.\n\nI would want to see the proposed \"read without locking, compute\nheavily and then take a lock and revalidate before writing it out,\nor fail\" done with a focus on not hurting the \"sane\" use pattern,\ni.e. making the \"revalidate\" part as light as possible. I however\ncannot seem to find any optimization opportunities other than the\n\"open, read and compare the checksum\". Two I thought and discarded\nare:\n\n - checking timestamp of the index file itself, and failing when it\n has changed (without opening or reading the checksum) is not\n good; it is optimizing for the wrong case, because we would need\n to check the checksum anyway when the timestamps match.\n\n - checking i-num of the index file itself, and failing when it has\n changed (without opening or reading the checksum) is not good,\n either. Creating \"index.lock\", writing to it and renaming it to\n \"index\", keeping the \"index\" during the whole period, would\n ensure that the index file that results with this single cycle\n would get a different i-num from the original, but if that is\n done twice or more, the same i-num can be reused and defeat the\n check.\n"}]}