Re: [PATCH 2/3] http-push: fix construction of hex value from path

From: Jeff King <peff@peff.net>
Date: Oct 3, 2017, 23:36 UTC
Message-ID: <20171003233638.fq6lgls2qsucfbn3@sigill.intra.peff.net>
In-Reply-To: <20171003225315.GE19555@aiede.mtv.corp.google.com>

On Tue, Oct 03, 2017 at 03:53:15PM -0700, Jonathan Nieder wrote:

Show 11 quoted lines

> Thomas Gummerer wrote:
> 
> > The get_oid_hex_from_objpath takes care of creating a oid from a
> > pathname.  It does this by memcpy'ing the first two bytes of the path to
> > the "hex" string, then skipping the '/', and then copying the rest of the
> > path to the "hex" string.  Currently it fails to increase the pointer to
> > the hex string, so the second memcpy invocation just mashes over what
> > was copied in the first one, and leaves the last two bytes in the string
> > uninitialized.
> 
> Wow.  The fix is obviously correct.

Agreed. This is brown-paper-bag worthy. Thanks, Thomas, for cleaning up my mess.

Show 5 quoted lines

> I think the problem is that when it fails, we end up thinking that
> there are *fewer* objects than are actually present remotely so the
> only ill effect is pushing too much.  So this should be observable in
> server logs (i.e. it is testable) but it's not a catastrophic failure
> which means it's harder to test than it would be otherwise.

And thank you, Jonathan, for this analysis. I had also wondered how such a frequently-triggered bug could have gone completely unnoticed, but this explains it.

> Moreover, this is in the webdav-based "dumb http" push code path,
> which I do not trust much at all.  I wonder if we could retire it
> completely (or at least provide an option to turn it off).

I would really like that, too. It has been the cause of a lot of pain when working with the smart code, and I am not at all surprised to find a bug of this magnitude lurking in it. I'd _hoped_ this could show that the system has been unusably broken for years, which would give us confidence to turn it off. :) But per your paragraph above, people could very easily still have been happily using it in the meantime.

-Peff