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

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

From
David Turner <dturner@twopensource.com>
Date
Apr 30, 2016, 01:01 UTC
Message-ID
<1461978133-13966-5-git-send-email-dturner@twopensource.com>
In-Reply-To
<1461978133-13966-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 81dfb4e..9eedf03 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1671,9 +1671,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 5 of 20 in “index-helper/watchman”
  1. 00/19 index-helper/watchmanDavid Turner, Apr 30, 2016
  2. 01/19 read-cache.c: fix constness of verify_hdr()David Turner, Apr 30, 2016
  3. 02/19 read-cache: allow to keep mmap'd memory after readingDavid Turner, Apr 30, 2016
  4. 03/19 index-helper: new daemon for caching index and related stuffDavid Turner, Apr 30, 2016
  5. 04/19 index-helper: add --strictDavid Turner, Apr 30, 2016
  6. 05/19 index-helper: log warningsDavid Turner, Apr 30, 2016
  7. 06/19 daemonize(): set a flag before exiting the main processDavid Turner, Apr 30, 2016
  8. 07/19 index-helper: add --detachDavid Turner, Apr 30, 2016
  9. 08/19 read-cache: add watchman 'WAMA' extensionDavid Turner, Apr 30, 2016
  10. 09/19 Add watchman support to reduce index refresh costDavid Turner, Apr 30, 2016
  11. 10/19 index-helper: use watchman to avoid refreshing index with lstat()David Turner, Apr 30, 2016
  12. 11/19 update-index: enable/disable watchman supportDavid Turner, Apr 30, 2016
  13. 12/19 unpack-trees: preserve index extensionsDavid Turner, Apr 30, 2016
  14. 13/19 watchman: add a config option to enable the extensionDavid Turner, Apr 30, 2016
  15. 14/19 index-helper: kill modeDavid Turner, Apr 30, 2016
  16. 15/19 index-helper: don't run if already runningDavid Turner, Apr 30, 2016
  17. 16/19 index-helper: autorun modeDavid Turner, Apr 30, 2016
  18. 17/19 index-helper: optionally automatically runDavid Turner, Apr 30, 2016
  19. 18/19 Add tracing to measure where most of the time is spentDavid Turner, Apr 30, 2016
  20. 19/19 untracked-cache: config optionDavid Turner, Apr 30, 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.