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

Re: [PATCH v2] Verify index file before we opportunistically update it

From
Junio C Hamano <gitster@pobox.com>
Date
Apr 9, 2014, 23:05 UTC
Message-ID
<xmqqwqeyxfwi.fsf@gitster.dls.corp.google.com>
In-Reply-To
<1397082869-24873-1-git-send-email-yiannis.marangos@gmail.com>
Yiannis Marangos <yiannis.marangos@gmail.com> writes:
> Before we proceed to "opportunistic update" we must verify that the
> current index file is the same as the one that we read before. There
> is a possible race if we don't do this:

Please somehow make it clear that the race is in general, and use of "git rebase" in this description is merely an example.

>   1. process A calls git-rebase
... or does anything that uses the index.
Show 7 quoted lines
>   2. process A applies 1st commit
>
>   3. process B calls git-status
>
>   4. process B reads index
>
>   5. process A applies 2nd commit
... or does anything that updates the index.
Show 5 quoted lines
>   6. process B takes the lock, then overwrites process A's changes.
>
>   7. process A applies 3rd commit
>
> As an end result the 3rd commit will have a revert of the 2nd commit.

And the missing piece is...? "When process B takes the lock, it needs to make sure that the index hasn't changed since it read at step 4."

> Signed-off-by: Yiannis Marangos <yiannis.marangos@gmail.com>
> ---

This is a place for you to describe how this version is different from the previous round. I am guessing that the only change is that you removed the unnecessary printf() from the top of if-able, but I didn't compare (nor read either versions carefully).

Show 22 quoted lines
>  cache.h      |  3 +++
>  read-cache.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 69 insertions(+), 11 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 107ac61..b0eedad 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -279,6 +279,7 @@ struct index_state {
>  		 initialized : 1;
>  	struct hashmap name_hash;
>  	struct hashmap dir_hash;
> +	unsigned char sha1[20];
>  };
>  
>  extern struct index_state the_index;
> @@ -456,6 +457,8 @@ extern int daemonize(void);
>  	} while (0)
>  
>  /* Initialize and use the cache information */
> +extern int verify_index_from(const struct index_state *, const char *path);
> +extern int verify_index(struct index_state *);

I do not think these should be extern; they are implementation details of the opportunistic update, and update-if-able is the only thing the outside world needs to know about, I think.

Show 12 quoted lines
>  extern int read_index(struct index_state *);
>  extern int read_index_preload(struct index_state *, const struct pathspec *pathspec);
>  extern int read_index_from(struct index_state *, const char *path);
> diff --git a/read-cache.c b/read-cache.c
> index ba13353..a49a441 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -14,6 +14,7 @@
>  #include "resolve-undo.h"
>  #include "strbuf.h"
>  #include "varint.h"
> +#include "git-compat-util.h"

An unnecessary change; we include "cache.h" as the very first thing in this file.

Show 6 quoted lines
> @@ -1321,6 +1322,59 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)
>  	return 0;
>  }
>  
> +/* This function verifies if index_state has the correct sha1 of an index file.
> + * Don't die if we have any other failure, just return 0. */
	/*
         * Please write multi-line comment
         * like this.
         */
> +int verify_index_from(const struct index_state *istate, const char *path)
> +{
Also, this does not have to be extern.  At least not yet.
Show 25 quoted lines
> +	int fd;
> +	struct stat st;
> +	struct cache_header *hdr;
> +	void *mmap_addr;
> +	size_t mmap_size;
> +
> +	if (!istate->initialized)
> +		return 0;
> +
> +	fd = open(path, O_RDONLY);
> +	if (fd < 0)
> +		return 0;
> +
> +	if (fstat(fd, &st))
> +		return 0;
> +
> +	/* file is too big */
> +	if (st.st_size > (size_t)st.st_size)
> +		return 0;
> +
> +	mmap_size = (size_t)st.st_size;
> +	if (mmap_size < sizeof(struct cache_header) + 20)
> +		return 0;
> +
> +	mmap_addr = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
Why PROT_WRITE?
Show 33 quoted lines
> +	close(fd);
> +	if (mmap_addr == MAP_FAILED)
> +		return 0;
> +
> +	hdr = mmap_addr;
> +	if (verify_hdr(hdr, mmap_size) < 0)
> +		goto unmap;
> +
> +	if (hashcmp(istate->sha1, (unsigned char *)hdr + mmap_size - 20))
> +		goto unmap;
> +
> +	munmap(mmap_addr, mmap_size);
> +	return 1;
> +
> +unmap:
> +	munmap(mmap_addr, mmap_size);
> +	return 0;
> +}
> +
> +int verify_index(struct index_state *istate)
> +{
> +	return verify_index_from(istate, get_index_file());
> +}
> +
>  static int read_index_extension(struct index_state *istate,
>  				const char *ext, void *data, unsigned long sz)
>  {
> @@ -1445,7 +1499,7 @@ int read_index_from(struct index_state *istate, const char *path)
>  	struct stat st;
>  	unsigned long src_offset;
>  	struct cache_header *hdr;
> -	void *mmap;
> +	void *mmap_addr;

This introduces noise to the patch to make it harder to review, and does not look a necessary change. Am I mistaken?

Show 68 quoted lines
>  	size_t mmap_size;
>  	struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
>  
> @@ -1468,15 +1522,16 @@ int read_index_from(struct index_state *istate, const char *path)
>  	if (mmap_size < sizeof(struct cache_header) + 20)
>  		die("index file smaller than expected");
>  
> -	mmap = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
> -	if (mmap == MAP_FAILED)
> +	mmap_addr = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
> +	if (mmap_addr == MAP_FAILED)
>  		die_errno("unable to map index file");
>  	close(fd);
>  
> -	hdr = mmap;
> +	hdr = mmap_addr;
>  	if (verify_hdr(hdr, mmap_size) < 0)
>  		goto unmap;
>  
> +	hashcpy(istate->sha1, (unsigned char *)hdr + mmap_size - 20);
>  	istate->version = ntohl(hdr->hdr_version);
>  	istate->cache_nr = ntohl(hdr->hdr_entries);
>  	istate->cache_alloc = alloc_nr(istate->cache_nr);
> @@ -1494,7 +1549,7 @@ int read_index_from(struct index_state *istate, const char *path)
>  		struct cache_entry *ce;
>  		unsigned long consumed;
>  
> -		disk_ce = (struct ondisk_cache_entry *)((char *)mmap + src_offset);
> +		disk_ce = (struct ondisk_cache_entry *)((char *)mmap_addr + src_offset);
>  		ce = create_from_disk(disk_ce, &consumed, previous_name);
>  		set_index_entry(istate, i, ce);
>  
> @@ -1512,21 +1567,21 @@ int read_index_from(struct index_state *istate, const char *path)
>  		 * in 4-byte network byte order.
>  		 */
>  		uint32_t extsize;
> -		memcpy(&extsize, (char *)mmap + src_offset + 4, 4);
> +		memcpy(&extsize, (char *)mmap_addr + src_offset + 4, 4);
>  		extsize = ntohl(extsize);
>  		if (read_index_extension(istate,
> -					 (const char *) mmap + src_offset,
> -					 (char *) mmap + src_offset + 8,
> +					 (const char *) mmap_addr + src_offset,
> +					 (char *) mmap_addr + src_offset + 8,
>  					 extsize) < 0)
>  			goto unmap;
>  		src_offset += 8;
>  		src_offset += extsize;
>  	}
> -	munmap(mmap, mmap_size);
> +	munmap(mmap_addr, mmap_size);
>  	return istate->cache_nr;
>  
>  unmap:
> -	munmap(mmap, mmap_size);
> +	munmap(mmap_addr, mmap_size);
>  	die("index file corrupt");
>  }
>  
> @@ -1779,7 +1834,7 @@ static int has_racy_timestamp(struct index_state *istate)
>  void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)
>  {
>  	if ((istate->cache_changed || has_racy_timestamp(istate)) &&
> -	    !write_index(istate, lockfile->fd))
> +	    verify_index(istate) && !write_index(istate, lockfile->fd))
>  		commit_locked_index(lockfile);
>  	else
>  		rollback_lock_file(lockfile);
Previous: Yiannis MarangosNext: Yiannis Marangos
Message 4 of 31 in “Verify index file before we opportunistically update it”
  1. Verify index file before we opportunistically update itYiannis Marangos, Apr 9, 2014
  2. Verify index file before we opportunistically update itYiannis Marangos, Apr 9, 2014
  3. Verify index file before we opportunistically update itYiannis Marangos, Apr 9, 2014
  4. Junio C HamanoApr 9, 2014
  5. Verify index file before we opportunistically update itYiannis Marangos, Apr 9, 2014
  6. Verify index file before we opportunistically update itYiannis Marangos, Apr 10, 2014
  7. Verify index file before we opportunistically update itYiannis Marangos, Apr 10, 2014
  8. Duy NguyenApr 10, 2014
  9. Yiannis MarangosApr 10, 2014
  10. Junio C HamanoApr 10, 2014
  11. Verify index file before we opportunistically update itYiannis Marangos, Apr 10, 2014
  12. 1/2 Add xpread() and xpwrite()Yiannis Marangos, Apr 10, 2014
  13. 2/2 Verify index file before we opportunistically update itYiannis Marangos, Apr 10, 2014
  14. Junio C HamanoApr 10, 2014
  15. Duy NguyenApr 11, 2014
  16. Junio C HamanoApr 11, 2014
  17. Junio C HamanoApr 11, 2014
  18. Yiannis MarangosApr 11, 2014
  19. Duy NguyenApr 12, 2014
  20. Junio C HamanoApr 12, 2014
  21. Junio C HamanoApr 12, 2014
  22. Duy NguyenApr 12, 2014
  23. Junio C HamanoApr 14, 2014
  24. Torsten BögershausenApr 11, 2014
  25. Yiannis MarangosApr 11, 2014
  26. Duy NguyenApr 11, 2014
  27. Junio C HamanoApr 10, 2014
  28. Yiannis MarangosApr 10, 2014
  29. 1/2 Add xpread()Yiannis Marangos, Apr 10, 2014
  30. Johannes SixtApr 10, 2014
  31. Junio C HamanoApr 10, 2014

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.