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

[Jeff King <peff@peff.net>]

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

> 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.

> 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