Hi, 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. > This breaks valgrind in t5540, although the test passes without > valgrind: [...] > Signed-off-by: Thomas Gummerer <t.gummerer@xxxxxxxxx> > --- > http-push.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Would it be straightforward to add a correctness test for this? It seems like this code path didn't work at all and no one noticed. This is the code path in http-push.c which says /* * NEEDSWORK: remote_ls() ignores info/refs on the remote side. But it * should _only_ heed the information from that file, instead of trying to * determine the refs from the remote file system (badly: it does not even * know about packed-refs). */ static void remote_ls(const char *path, int flags, 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. 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). > diff --git a/http-push.c b/http-push.c > index e4c9b065ce..e9a01ec4da 100644 > --- a/http-push.c > +++ b/http-push.c > @@ -1018,7 +1018,7 @@ static int get_oid_hex_from_objpath(const char *path, struct object_id *oid) > memcpy(hex, path, 2); > path += 2; > path++; /* skip '/' */ > - memcpy(hex, path, GIT_SHA1_HEXSZ - 2); > + memcpy(hex + 2, path, GIT_SHA1_HEXSZ - 2); > > return get_oid_hex(hex, oid); Thanks, Jonathan
