Re: git bugs

["Ben Lynn" <benlynn@gmail.com>]

I hadn't thought of exploiting the fact that the SHA1 of an empty file
is fixed. Nice! I believe I can prove there are no races now.
Incidentally, this is how I first found the bug: I was trying to prove
what git did worked.

I still prefer a per-entry flag solution (I suspect it's faster, and
the proof is easier), but that's too much work.

-Ben

On Tue, Jun 10, 2008 at 10:44 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
>
> On Tue, 10 Jun 2008, Ben Lynn wrote:
>>
>> Unfortunately, the solution isn't perfect. Try this:
>
> Heh.
>
> That's just because our "smudge_racily_clean_entry()" uses 0 as the magic
> smudging size.
>
> You can fix this multiple ways. One would be to pick another size that is
> simply less likely (eg ~0 instead), which leaves the theoretical race, and
> just makes it practically impossible to hit (not that I think it's very
> practical to hit already).
>
> The other approach is to know that an empty blob always has a very
> specific SHA1. Here's an trial patch.
>
>                Linus
>
> ---
>  read-cache.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 8e5fbb6..f83de8c 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -138,6 +138,16 @@ static int ce_modified_check_fs(struct cache_entry *ce, struct stat *st)
>        return 0;
>  }
>
> +static int is_empty_blob_sha1(const unsigned char *sha1)
> +{
> +       static const unsigned char empty_blob_sha1[20] = {
> +               0xe6,0x9d,0xe2,0x9b,0xb2,0xd1,0xd6,0x43,0x4b,0x8b,
> +               0x29,0xae,0x77,0x5a,0xd8,0xc2,0xe4,0x8c,0x53,0x91
> +       };
> +
> +       return !hashcmp(sha1, empty_blob_sha1);
> +}
> +
>  static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
>  {
>        unsigned int changed = 0;
> @@ -193,6 +203,12 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
>        if (ce->ce_size != (unsigned int) st->st_size)
>                changed |= DATA_CHANGED;
>
> +       /* Racily smudged entry? */
> +       if (!ce->ce_size) {
> +               if (!is_empty_blob_sha1(ce->sha1))
> +                       changed |= DATA_CHANGED;
> +       }
> +
>        return changed;
>  }
>
>