Re: [PATCH] use a hash of the lock token as the suffix for PUT/MOVE

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 14, 2009, 08:34 UTC
Message-ID: <7vskmhwh6m.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <49967321.10800@gmail.com>

Tay Ray Chuan <rctay89@gmail.com> writes:

Show 19 quoted lines

> After 753bc91 ("Remove the requirement opaquelocktoken uri scheme"),
> lock tokens are in the URI forms in which they are received from the
> server, eg. 'opaquelocktoken:', 'urn:uuid:'.
>
> However, "start_put" (and consequently "start_move"), which attempts to
> create a unique temporary file using the UUID of the lock token,
> inadvertently uses the lock token in its URI form. These file
> operations on the server may not be successful (specifically, in
> Windows), due to the colon ':' character from the URI form of the lock
> token in the file path.
>
> This patch uses a hash of the lock token instead, guaranteeing only
> "safe" characters (a-f, 0-9) are used in the file path.
>
> The token's hash is generated when the lock token is received from the
> server in handle_new_lock_ctx, minimizing the number of times of
> hashing.
>
> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>

Thanks, very clearly written.

Show 9 quoted lines

> diff --git a/http-push.c b/http-push.c
> index eefd64c..0a252dd 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -153,6 +153,7 @@ struct remote_lock
>  	char *url;
>  	char *owner;
>  	char *token;
> +	char *token_sha1_hex;

At this point, this new field is only used as a unique suffix, and "sha1_hex" is a implementation detail of the mechanism to guarantee the uniqueness. Naming things for what they are is preferred over naming things for how they are crafted. Hence:

	char tmpfile_suffix[41];

would be a better definition here.

Show 6 quoted lines

> @@ -558,7 +559,7 @@ static void start_put(struct transfer_request *request)
>
>  	append_remote_object_url(&buf, remote->url, hex, 0);
>  	strbuf_addstr(&buf, "_");
> -	strbuf_addstr(&buf, request->lock->token);
> +	strbuf_addstr(&buf, request->lock->token_sha1_hex, 41);

And replace these two strbuf_addstr() with:

	strbuf_add(&buf, request->lock->tmpfile_suffix, 41);

Show 19 quoted lines

> @@ -1130,6 +1131,8 @@ static void handle_lockprop_ctx(struct xml_ctx *ctx, int tag_closed)
>  static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed)
>  {
>  	struct remote_lock *lock = (struct remote_lock *)ctx->userData;
> +	git_SHA_CTX sha_ctx;
> +	unsigned char lock_token_sha1[20];
>
>  	if (tag_closed && ctx->cdata) {
>  		if (!strcmp(ctx->name, DAV_ACTIVELOCK_OWNER)) {
> @@ -1142,6 +1145,12 @@ static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed)
>  		} else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TOKEN)) {
>  			lock->token = xmalloc(strlen(ctx->cdata) + 1);
>  			strcpy(lock->token, ctx->cdata);
> +
> +			git_SHA1_Init(&sha_ctx);
> +			git_SHA1_Update(&sha_ctx, lock->token, strlen(lock->token));
> +			git_SHA1_Final(lock_token_sha1, &sha_ctx);
> +
> +			lock->token_sha1_hex = sha1_to_hex(lock_token_sha1);

The last one is wrong because string returned by sha1_to_hex() is volatile.

	lock->tmpfile_suffix[0] = '_';
        memcpy(lock->tmpfile_suffix + 1, sha1_to_hex(sha1));

Other than that, I think this is a good patch.