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

Thomas Gummerer wrote:

quoted

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