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
