git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v9 04/19] index-helper: add --strict

From
David Turner <dturner@twopensource.com>
Date
May 9, 2016, 20:48 UTC
Message-ID
<1462826929-7567-5-git-send-email-dturner@twopensource.com>
In-Reply-To
<1462826929-7567-1-git-send-email-dturner@twopensource.com>
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

There are "holes" in the index-helper approach because the shared memory is not verified again by git. If $USER is compromised, shared memory could be modified. But anyone who could do this could already modify $GIT_DIR/index. A more realistic risk is some bugs in index-helper that produce corrupt shared memory. --strict is added to avoid that.

Strictly speaking there's still a very small gap where corrupt shared memory could still be read by git: after we write the trailing SHA-1 in the shared memory (thus signaling "this shm is ready") and before verify_shm() detects an error.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: David Turner <dturner@twopensource.com>
---
 Documentation/git-index-helper.txt |  9 +++++++
 cache.h                            |  1 +
 index-helper.c                     | 48 ++++++++++++++++++++++++++++++++++++++
 read-cache.c                       |  9 ++++---
 4 files changed, 64 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-index-helper.txt b/Documentation/git-index-helper.txt
index f892184..1f92c89 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -25,6 +25,15 @@ OPTIONS
 	Exit if the cached index is not accessed for `<n>`
 	seconds. Specify 0 to wait forever. Default is 600.
 
+--strict::
+--no-strict::
+	Strict mode makes index-helper verify the shared memory after
+	it's created. If the result does not match what's read from
+	$GIT_DIR/index, the shared memory is destroyed. This makes
+	index-helper take more than double the amount of time required
+	for reading an index, but because it will happen in the
+	background, it's not noticable. `--strict` is enabled by default.
+
 NOTES
 -----
 
diff --git a/cache.h b/cache.h
index 2d7af6f..6cb0d02 100644
--- a/cache.h
+++ b/cache.h
@@ -345,6 +345,7 @@ struct index_state {
 		  * on it.
 		  */
 		 to_shm : 1,
+		 always_verify_trailing_sha1 : 1,
 		 initialized : 1;
 	struct hashmap name_hash;
 	struct hashmap dir_hash;
diff --git a/index-helper.c b/index-helper.c
index b9443f4..bc7e110 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -17,6 +17,7 @@ struct shm {
 
 static struct shm shm_index;
 static struct shm shm_base_index;
+static int to_verify = 1;
 
 static void release_index_shm(struct shm *is)
 {
@@ -114,11 +115,56 @@ static void share_index(struct index_state *istate, struct shm *is)
 	hashcpy((unsigned char *)new_mmap + istate->mmap_size - 20, is->sha1);
 }
 
+static int verify_shm(void)
+{
+	int i;
+	struct index_state istate;
+	memset(&istate, 0, sizeof(istate));
+	istate.always_verify_trailing_sha1 = 1;
+	istate.to_shm = 1;
+	i = read_index(&istate);
+	if (i != the_index.cache_nr)
+		goto done;
+	for (; i < the_index.cache_nr; i++) {
+		struct cache_entry *base, *ce;
+		/* namelen is checked separately */
+		const unsigned int ondisk_flags =
+			CE_STAGEMASK | CE_VALID | CE_EXTENDED_FLAGS;
+		unsigned int ce_flags, base_flags, ret;
+		base = the_index.cache[i];
+		ce = istate.cache[i];
+		if (ce->ce_namelen != base->ce_namelen ||
+		    strcmp(ce->name, base->name)) {
+			warning("mismatch at entry %d", i);
+			break;
+		}
+		ce_flags = ce->ce_flags;
+		base_flags = base->ce_flags;
+		/* only on-disk flags matter */
+		ce->ce_flags   &= ondisk_flags;
+		base->ce_flags &= ondisk_flags;
+		ret = memcmp(&ce->ce_stat_data, &base->ce_stat_data,
+			     offsetof(struct cache_entry, name) -
+			     offsetof(struct cache_entry, ce_stat_data));
+		ce->ce_flags = ce_flags;
+		base->ce_flags = base_flags;
+		if (ret) {
+			warning("mismatch at entry %d", i);
+			break;
+		}
+	}
+done:
+	discard_index(&istate);
+	return i == the_index.cache_nr;
+}
+
 static void share_the_index(void)
 {
 	if (the_index.split_index && the_index.split_index->base)
 		share_index(the_index.split_index->base, &shm_base_index);
 	share_index(&the_index, &shm_index);
+	if (to_verify && !verify_shm())
+		cleanup_shm();
 	discard_index(&the_index);
 }
 
@@ -247,6 +293,8 @@ int main(int argc, char **argv)
 	struct option options[] = {
 		OPT_INTEGER(0, "exit-after", &idle_in_seconds,
 			    N_("exit if not used after some seconds")),
+		OPT_BOOL(0, "strict", &to_verify,
+			 "verify shared memory after creating"),
 		OPT_END()
 	};
 
diff --git a/read-cache.c b/read-cache.c
index 1a5a03d..d7849d6 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1674,9 +1674,12 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 
 	istate->mmap = mmap;
 	istate->mmap_size = mmap_size;
-	if (try_shm(istate) &&
-	    verify_hdr(istate->mmap, istate->mmap_size) < 0)
-		goto unmap;
+	if (try_shm(istate)) {
+		if (verify_hdr(istate->mmap, istate->mmap_size) < 0)
+			goto unmap;
+	} else if (istate->always_verify_trailing_sha1 &&
+		   verify_hdr(istate->mmap, istate->mmap_size) < 0)
+			goto unmap;
 	hdr = mmap = istate->mmap;
 	mmap_size = istate->mmap_size;
 	if (!istate->keep_mmap)

-- 
2.4.2.767.g62658d5-twtrsrc
Previous: David TurnerNext: David Turner
Message 10 of 39 in “index-helper/watchman”
  1. 00/19 index-helper/watchmanDavid Turner, May 9, 2016
  2. 01/19 read-cache.c: fix constness of verify_hdr()David Turner, May 9, 2016
  3. 02/19 read-cache: allow to keep mmap'd memory after readingDavid Turner, May 9, 2016
  4. 03/19 index-helper: new daemon for caching index and related stuffDavid Turner, May 9, 2016
  5. SZEDER GáborMay 10, 2016
  6. Duy NguyenMay 10, 2016
  7. SZEDER GáborMay 10, 2016
  8. Duy NguyenMay 10, 2016
  9. David TurnerMay 10, 2016
  10. 04/19 index-helper: add --strictDavid Turner, May 9, 2016
  11. 05/19 index-helper: log warningsDavid Turner, May 9, 2016
  12. 06/19 daemonize(): set a flag before exiting the main processDavid Turner, May 9, 2016
  13. 07/19 index-helper: add --detachDavid Turner, May 9, 2016
  14. 08/19 read-cache: add watchman 'WAMA' extensionDavid Turner, May 9, 2016
  15. 09/19 watchman: support watchman to reduce index refresh costDavid Turner, May 9, 2016
  16. 10/19 index-helper: use watchman to avoid refreshing index with lstat()David Turner, May 9, 2016
  17. 11/19 update-index: enable/disable watchman supportDavid Turner, May 9, 2016
  18. 12/19 unpack-trees: preserve index extensionsDavid Turner, May 9, 2016
  19. 13/19 watchman: add a config option to enable the extensionDavid Turner, May 9, 2016
  20. 14/19 index-helper: kill modeDavid Turner, May 9, 2016
  21. 15/19 index-helper: don't run if already runningDavid Turner, May 9, 2016
  22. 16/19 index-helper: autorun modeDavid Turner, May 9, 2016
  23. 17/19 index-helper: optionally automatically runDavid Turner, May 9, 2016
  24. 18/19 trace: measure where the time is spent in the index-heavy operationsDavid Turner, May 9, 2016
  25. 19/19 untracked-cache: config optionDavid Turner, May 9, 2016
  26. Junio C HamanoMay 9, 2016
  27. David TurnerMay 9, 2016
  28. Junio C HamanoMay 9, 2016
  29. Junio C HamanoMay 9, 2016
  30. David TurnerMay 9, 2016
  31. Ramsay JonesMay 9, 2016
  32. Duy NguyenMay 10, 2016
  33. Duy NguyenMay 10, 2016
  34. Dennis KaarsemakerMay 10, 2016
  35. Dennis KaarsemakerMay 10, 2016
  36. Dennis KaarsemakerMay 10, 2016
  37. Ramsay JonesMay 10, 2016
  38. Junio C HamanoMay 10, 2016
  39. Ramsay JonesMay 10, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.