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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 3, 2017, 22:53 UTC
Message-ID: <20171003225315.GE19555@aiede.mtv.corp.google.com>
In-Reply-To: <20171003195713.13395-3-t.gummerer@gmail.com>

Hi,

Thomas Gummerer wrote:

Show 7 quoted lines

> 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@gmail.com>
> ---
>  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).

Show 12 quoted lines

> 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