Re: [PATCH 2/3] http-push: fix construction of hex value from path
- Date: Tue, 3 Oct 2017 19:36:38 -0400
- From: Jeff King <peff@xxxxxxxx>
- Subject: Re: [PATCH 2/3] http-push: fix construction of hex value from path
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
> 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.