{"thread":{"id":"17787","subject":"[PATCH] use a hash of the lock token as the suffix for PUT/MOVE","startedAt":"2009-02-14T07:30:41Z","lastAt":"2009-02-14T09:53:40Z","messageCount":4,"participants":["Tay Ray Chuan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"104607","messageId":"49967321.10800@gmail.com","threadId":"17787","inReplyTo":null,"subject":"[PATCH] use a hash of the lock token as the suffix for PUT/MOVE","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-02-14T07:30:41Z","receivedAt":"2009-02-14T07:30:41Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"After 753bc91 (\"Remove the requirement opaquelocktoken uri scheme\"),\nlock tokens are in the URI forms in which they are received from the\nserver, eg. 'opaquelocktoken:', 'urn:uuid:'.\n\nHowever, \"start_put\" (and consequently \"start_move\"), which attempts to\ncreate a unique temporary file using the UUID of the lock token,\ninadvertently uses the lock token in its URI form. These file\noperations on the server may not be successful (specifically, in\nWindows), due to the colon ':' character from the URI form of the lock\ntoken in the file path.\n\nThis patch uses a hash of the lock token instead, guaranteeing only\n\"safe\" characters (a-f, 0-9) are used in the file path.\n\nThe token's hash is generated when the lock token is received from the\nserver in handle_new_lock_ctx, minimizing the number of times of\nhashing.\n\nSigned-off-by: Tay Ray Chuan <rctay89@gmail.com>\n---\n\nThis patch is a result of the discussion on \"[PATCH] use lock token in\nnon-URI form in start_put\"; you can read it at\nhttp://kerneltrap.org/mailarchive/git/2009/2/7/4922094.\n\nThe decision to use a hash of the token is so that one can avoid\nhandling the URI scheme of the lock token, which may be a deeply\nnested URI, or (the more likely scenario) contain \"unsafe\" characters\nfor a file name, such as colons, slashes and spaces.\n\n http-push.c          |   11 ++++++++++-\n t/t5540-http-push.sh |    7 +++++++\n 2 files changed, 17 insertions(+), 1 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex eefd64c..0a252dd 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -153,6 +153,7 @@ struct remote_lock\n \tchar *url;\n \tchar *owner;\n \tchar *token;\n+\tchar *token_sha1_hex;\n \ttime_t start_time;\n \tlong timeout;\n \tint refreshing;\n@@ -558,7 +559,7 @@ static void start_put(struct transfer_request *request)\n\n \tappend_remote_object_url(&buf, remote->url, hex, 0);\n \tstrbuf_addstr(&buf, \"_\");\n-\tstrbuf_addstr(&buf, request->lock->token);\n+\tstrbuf_addstr(&buf, request->lock->token_sha1_hex);\n \trequest->url = strbuf_detach(&buf, NULL);\n\n \tslot = get_active_slot();\n@@ -1130,6 +1131,8 @@ static void handle_lockprop_ctx(struct xml_ctx *ctx, int tag_closed)\n static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed)\n {\n \tstruct remote_lock *lock = (struct remote_lock *)ctx->userData;\n+\tgit_SHA_CTX sha_ctx;\n+\tunsigned char lock_token_sha1[20];\n\n \tif (tag_closed && ctx->cdata) {\n \t\tif (!strcmp(ctx->name, DAV_ACTIVELOCK_OWNER)) {\n@@ -1142,6 +1145,12 @@ static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed)\n \t\t} else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TOKEN)) {\n \t\t\tlock->token = xmalloc(strlen(ctx->cdata) + 1);\n \t\t\tstrcpy(lock->token, ctx->cdata);\n+\n+\t\t\tgit_SHA1_Init(&sha_ctx);\n+\t\t\tgit_SHA1_Update(&sha_ctx, lock->token, strlen(lock->token));\n+\t\t\tgit_SHA1_Final(lock_token_sha1, &sha_ctx);\n+\n+\t\t\tlock->token_sha1_hex = sha1_to_hex(lock_token_sha1);\n \t\t}\n \t}\n }\ndiff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh\nindex c236b5e..11b3432 100755\n--- a/t/t5540-http-push.sh\n+++ b/t/t5540-http-push.sh\n@@ -94,6 +94,13 @@ test_expect_success 'MKCOL sends directory names with trailing slashes' '\n\n '\n\n+test_expect_success 'PUT and MOVE sends object to URLs with SHA-1 hash suffix' '\n+\n+\tgrep -P \"\\\"(?:PUT|MOVE) .+objects/[\\da-z]{2}/[\\da-z]{38}_[\\da-z\\-]{40} HTTP/[0-9.]+\\\" 20\\d\" \\\n+\t\t< \"$HTTPD_ROOT_PATH\"/access.log\n+\n+'\n+\n stop_httpd\n\n test_done\n-- \n1.6.1.2.278.g9a9e.dirty\n"},{"id":"104612","messageId":"7vskmhwh6m.fsf@gitster.siamese.dyndns.org","threadId":"17787","inReplyTo":"49967321.10800@gmail.com","subject":"Re: [PATCH] use a hash of the lock token as the suffix for PUT/MOVE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-14T08:34:09Z","receivedAt":"2009-02-14T08:34:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan <rctay89@gmail.com> writes:\n\n> After 753bc91 (\"Remove the requirement opaquelocktoken uri scheme\"),\n> lock tokens are in the URI forms in which they are received from the\n> server, eg. 'opaquelocktoken:', 'urn:uuid:'.\n>\n> However, \"start_put\" (and consequently \"start_move\"), which attempts to\n> create a unique temporary file using the UUID of the lock token,\n> inadvertently uses the lock token in its URI form. These file\n> operations on the server may not be successful (specifically, in\n> Windows), due to the colon ':' character from the URI form of the lock\n> token in the file path.\n>\n> This patch uses a hash of the lock token instead, guaranteeing only\n> \"safe\" characters (a-f, 0-9) are used in the file path.\n>\n> The token's hash is generated when the lock token is received from the\n> server in handle_new_lock_ctx, minimizing the number of times of\n> hashing.\n>\n> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>\n\nThanks, very clearly written.\n\n> diff --git a/http-push.c b/http-push.c\n> index eefd64c..0a252dd 100644\n> --- a/http-push.c\n> +++ b/http-push.c\n> @@ -153,6 +153,7 @@ struct remote_lock\n>  \tchar *url;\n>  \tchar *owner;\n>  \tchar *token;\n> +\tchar *token_sha1_hex;\n\nAt this point, this new field is only used as a unique suffix, and\n\"sha1_hex\" is a implementation detail of the mechanism to guarantee the\nuniqueness.  Naming things for what they are is preferred over naming\nthings for how they are crafted.  Hence:\n\n\tchar tmpfile_suffix[41];\n\nwould be a better definition here.\n\n> @@ -558,7 +559,7 @@ static void start_put(struct transfer_request *request)\n>\n>  \tappend_remote_object_url(&buf, remote->url, hex, 0);\n>  \tstrbuf_addstr(&buf, \"_\");\n> -\tstrbuf_addstr(&buf, request->lock->token);\n> +\tstrbuf_addstr(&buf, request->lock->token_sha1_hex, 41);\n\nAnd replace these two strbuf_addstr() with:\n\n\tstrbuf_add(&buf, request->lock->tmpfile_suffix, 41);\n\n> @@ -1130,6 +1131,8 @@ static void handle_lockprop_ctx(struct xml_ctx *ctx, int tag_closed)\n>  static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed)\n>  {\n>  \tstruct remote_lock *lock = (struct remote_lock *)ctx->userData;\n> +\tgit_SHA_CTX sha_ctx;\n> +\tunsigned char lock_token_sha1[20];\n>\n>  \tif (tag_closed && ctx->cdata) {\n>  \t\tif (!strcmp(ctx->name, DAV_ACTIVELOCK_OWNER)) {\n> @@ -1142,6 +1145,12 @@ static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed)\n>  \t\t} else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TOKEN)) {\n>  \t\t\tlock->token = xmalloc(strlen(ctx->cdata) + 1);\n>  \t\t\tstrcpy(lock->token, ctx->cdata);\n> +\n> +\t\t\tgit_SHA1_Init(&sha_ctx);\n> +\t\t\tgit_SHA1_Update(&sha_ctx, lock->token, strlen(lock->token));\n> +\t\t\tgit_SHA1_Final(lock_token_sha1, &sha_ctx);\n> +\n> +\t\t\tlock->token_sha1_hex = sha1_to_hex(lock_token_sha1);\n\nThe last one is wrong because string returned by sha1_to_hex() is\nvolatile.\n\n\tlock->tmpfile_suffix[0] = '_';\n        memcpy(lock->tmpfile_suffix + 1, sha1_to_hex(sha1));\n\nOther than that, I think this is a good patch.\n"},{"id":"104620","messageId":"4996944E.9000104@gmail.com","threadId":"17787","inReplyTo":"7vskmhwh6m.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] use a hash of the lock token as the suffix for PUT/MOVE","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-02-14T09:52:14Z","receivedAt":"2009-02-14T09:52:14Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"After 753bc91 (\"Remove the requirement opaquelocktoken uri scheme\"),\nlock tokens are in the URI forms in which they are received from the\nserver, eg. 'opaquelocktoken:', 'urn:uuid:'.\n\nHowever, \"start_put\" (and consequently \"start_move\"), which attempts to\ncreate a unique temporary file using the UUID of the lock token,\ninadvertently uses the lock token in its URI form. These file\noperations on the server may not be successful (specifically, in\nWindows), due to the colon ':' character from the URI form of the lock\ntoken in the file path.\n\nThis patch uses a hash of the lock token instead, guaranteeing only\n\"safe\" characters (a-f, 0-9) are used in the file path.\n\nThe token's hash is generated when the lock token is received from the\nserver in handle_new_lock_ctx, minimizing the number of times of\nhashing.\n\nSigned-off-by: Tay Ray Chuan <rctay89@gmail.com>\n---\n\nThis patch is a result of the discussion on \"[PATCH] use lock token in\nnon-URI form in start_put\"; you can read it at\nhttp://kerneltrap.org/mailarchive/git/2009/2/7/4922094.\n\nThe decision to use a hash of the token is so that one can avoid\nhandling the URI scheme of the lock token, which may be a deeply\nnested URI, or (the more likely scenario) contain \"unsafe\" characters\nfor a file name, such as colons, slashes and spaces.\n\n* use tmpfile_suffix instead of token_sha1_hex in remote_lock, as\nsuggested by Junio, and absorb '_' as a result\n* memcpy string from sha1_to_hex to tmpfile_suffix\n\n http-push.c          |   13 +++++++++++--\n t/t5540-http-push.sh |    7 +++++++\n 2 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex eefd64c..edbff58 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -153,6 +153,7 @@ struct remote_lock\n \tchar *url;\n \tchar *owner;\n \tchar *token;\n+\tchar tmpfile_suffix[41];\n \ttime_t start_time;\n \tlong timeout;\n \tint refreshing;\n@@ -557,8 +558,7 @@ static void start_put(struct transfer_request *request)\n \trequest->dest = strbuf_detach(&buf, NULL);\n\n \tappend_remote_object_url(&buf, remote->url, hex, 0);\n-\tstrbuf_addstr(&buf, \"_\");\n-\tstrbuf_addstr(&buf, request->lock->token);\n+\tstrbuf_add(&buf, request->lock->tmpfile_suffix, 41);\n \trequest->url = strbuf_detach(&buf, NULL);\n\n \tslot = get_active_slot();\n@@ -1130,6 +1130,8 @@ static void handle_lockprop_ctx(struct xml_ctx *ctx, int tag_closed)\n static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed)\n {\n \tstruct remote_lock *lock = (struct remote_lock *)ctx->userData;\n+\tgit_SHA_CTX sha_ctx;\n+\tunsigned char lock_token_sha1[20];\n\n \tif (tag_closed && ctx->cdata) {\n \t\tif (!strcmp(ctx->name, DAV_ACTIVELOCK_OWNER)) {\n@@ -1142,6 +1144,13 @@ static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed)\n \t\t} else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TOKEN)) {\n \t\t\tlock->token = xmalloc(strlen(ctx->cdata) + 1);\n \t\t\tstrcpy(lock->token, ctx->cdata);\n+\n+\t\t\tgit_SHA1_Init(&sha_ctx);\n+\t\t\tgit_SHA1_Update(&sha_ctx, lock->token, strlen(lock->token));\n+\t\t\tgit_SHA1_Final(lock_token_sha1, &sha_ctx);\n+\n+\t\t\tlock->tmpfile_suffix[0] = '_';\n+\t\t\tmemcpy(lock->tmpfile_suffix + 1, sha1_to_hex(lock_token_sha1), 40);\n \t\t}\n \t}\n }\ndiff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh\nindex c236b5e..11b3432 100755\n--- a/t/t5540-http-push.sh\n+++ b/t/t5540-http-push.sh\n@@ -94,6 +94,13 @@ test_expect_success 'MKCOL sends directory names with trailing slashes' '\n\n '\n\n+test_expect_success 'PUT and MOVE sends object to URLs with SHA-1 hash suffix' '\n+\n+\tgrep -P \"\\\"(?:PUT|MOVE) .+objects/[\\da-z]{2}/[\\da-z]{38}_[\\da-z\\-]{40} HTTP/[0-9.]+\\\" 20\\d\" \\\n+\t\t< \"$HTTPD_ROOT_PATH\"/access.log\n+\n+'\n+\n stop_httpd\n\n test_done\n-- \n1.6.1.2.278.g9a9e.dirty\n"},{"id":"104621","messageId":"be6fef0d0902140153i568fd1ecuff79635341aa9576@mail.gmail.com","threadId":"17787","inReplyTo":"7vskmhwh6m.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] use a hash of the lock token as the suffix for PUT/MOVE","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-02-14T09:53:40Z","receivedAt":"2009-02-14T09:53:40Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Sat, Feb 14, 2009 at 4:34 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Other than that, I think this is a good patch.\n>\n\nthanks for the encouraging words, I've sent in the modified patch already.\n\n-- \nCheers,\nRay Chuan\n"}]}