On 2014-04-10 21.28, Junio C Hamano wrote:
> Yiannis Marangos <yiannis.marangos@xxxxxxxxx> writes:
>
>> + n = xpread(fd, sha1, 20, st.st_size - 20);
>> + if (n != 20)
>> + goto out;
>
> I think it is possible for pread(2) to give you a short-read.
>
> The existing callers of emulated mmap and index-pack are prepared to
> handle a short-read correctly, but I do not think this code does.
>
> I'll queue this instead in the meantime.
>
> -- >8 --
> From: Yiannis Marangos <yiannis.marangos@xxxxxxxxx>
> Date: Thu, 10 Apr 2014 21:31:21 +0300
> Subject: [PATCH] read-cache.c: verify index file before we opportunistically update it
>
> Before we proceed to opportunistically update the index (often done
> by an otherwise read-only operation like "git status" and "git diff"
> that internally refreshes the index), we must verify that the
> current index file is the same as the one that we read earlier
> before we took the lock on it, in order to avoid a possible race.
>
> In the example below git-status does "opportunistic update" and
> git-rebase updates the index, but the race can happen in general.
I'm not sure if we need the second or third commit of process A at all.
My understanding is that the simplified version will have problems as well:
1. process A calls git-rebase (or does anything that updates the index)
2. process change
3. process B calls git-status (or does anything that updates the index)
4. process B reads the index file into memory
5. process change
6. process A applies a commit:
- read the index into memory
- take the lock
- update the index file on disc
- release the lock
7. process change
8. process B applies a commit:
- take the lock
- update the index in memory and write the index file to disc
- release the lock
Now process B has overwritten the commit from process A, which is wrong.
The new code works like this:
8. process B applies a commit:
- take the lock
- verifies tha the index file on disc has the same sha as the one read before
# And if not: What do we do? die() or retry() ?
- update the index file on disc
- release the lock
[]
>
> +static int verify_index_from(const struct index_state *istate, const char *path)
> +{
> + int fd;
> + ssize_t n;
> + struct stat st;
> + unsigned char sha1[20];
> +
> + if (!istate->initialized)
> + return 0;
> +
> + fd = open(path, O_RDONLY);
> + if (fd < 0)
> + return 0;
> +
> + if (fstat(fd, &st))
> + goto out;
> +
> + if (st.st_size < sizeof(struct cache_header) + 20)
> + goto out;
> +
> + n = pread_in_full(fd, sha1, 20, st.st_size - 20);
Minor :
What is the advantage of pread() against a lseek()/read_in_full() combo?
fd is opened and used only in one thread.
Introducing pread()/ pread_in_full() could be done in an other commit,
or do I miss something ?
> + if (n != 20)
> + goto out;
> +static int verify_index(const struct index_state *istate)
> +{
> + return verify_index_from(istate, get_index_file());
> +}
> +
Minor:
Do we really need the wrapper function verify_index_from()?
It seems as if the whole code from verify_index_from() could go into
verify_index(), which will call get_index_file()
> static int has_racy_timestamp(struct index_state *istate)
> {
> int entries = istate->cache_nr;
> @@ -1766,7 +1811,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);
> diff --git a/wrapper.c b/wrapper.c
> index 5b3c7fc..bc1bfb8 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -232,6 +232,26 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)
> return total;
> }
>
> +ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset)
> +{
> + char *p = buf;
> + ssize_t total = 0;
> +
> + while (count > 0) {
> + ssize_t loaded = xpread(fd, p, count, offset);
> + if (loaded < 0)
> + return -1;
> + if (loaded == 0)
> + return total;
> + count -= loaded;
> + p += loaded;
> + total += loaded;
> + offset += loaded;
> + }
> +
> + return total;
> +}
> +
> int xdup(int fd)
> {
> int ret = dup(fd);
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html