{"thread":{"id":"17634","subject":"[PATCH] use lock token in non-URI form in start_put","startedAt":"2009-02-07T19:27:53Z","lastAt":"2009-02-08T03:28:27Z","messageCount":7,"participants":["Tay Ray Chuan","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"103645","messageId":"498DE0B9.6080603@gmail.com","threadId":"17634","inReplyTo":null,"subject":"[PATCH] use lock token in non-URI form in start_put","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-02-07T19:27:53Z","receivedAt":"2009-02-07T19:27:53Z","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:', '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 ensures that the lock token sans the URI scheme is used\ninstead in \"start_put\".\n\nTo do this, the \"start_put\" gets the position of ':', which is used to\nseparate the URI scheme from the part, eg. \":\". In addition,\nstart_put uses the last position of ':', since URIs with component\nURIs are possible, eg. \"urn:uuid:\" One can be sure that the lock token\nwill always contain the UUID and be in URI form, due to RFC 2518, or\nits successor RFC 4918 (see\nhttp://www.webdav.org/specs/rfc4918.html#ELEMENT_locktoken).\n\nSigned-off-by: Tay Ray Chuan \n\nSigned-off-by: Tay Ray Chuan \n---\n http-push.c | 2 +-\n t/t5540-http-push.sh | 7 +++++++\n 2 files changed, 8 insertions(+), 1 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex eefd64c..bd8f372 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -558,7 +558,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, strrchr(request->lock->token, ':') + 1);\n \trequest->url = strbuf_detach(&buf, NULL);\n\n \tslot = get_active_slot();\ndiff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh\nindex c236b5e..268b2d4 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 in non-URI form' '\n+\n+\tgrep -P \"\\\"(?:PUT|MOVE) .+objects/[\\da-z]{2}/[\\da-z]{38}_[\\da-z\\-]{36} HTTP/[0-9.]+\\\" 20\\d\" \\\n+\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":"103647","messageId":"alpine.DEB.1.00.0902072114360.10279@pacific.mpi-cbg.de","threadId":"17634","inReplyTo":"498DE0B9.6080603@gmail.com","subject":"Re: [PATCH] use lock token in non-URI form in start_put","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-07T20:20:54Z","receivedAt":"2009-02-07T20:20:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 8 Feb 2009, Tay Ray Chuan wrote:\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:', '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\nIf it is a prefix that happens to be part of the URI, but must not be used \nby the client code as a lock token, would it not be better to store the \ntoken in lock->token to begin with?\n\n> To do this, the \"start_put\" gets the position of ':', which is used to\n> separate the URI scheme from the part, eg. \":\". In addition,\n> start_put uses the last position of ':', since URIs with component\n> URIs are possible, eg. \"urn:uuid:\" One can be sure that the lock token\n> will always contain the UUID and be in URI form, due to RFC 2518, or\n> its successor RFC 4918 (see\n> http://www.webdav.org/specs/rfc4918.html#ELEMENT_locktoken).\n> \n> Signed-off-by: Tay Ray Chuan \n> \n> Signed-off-by: Tay Ray Chuan \n> ---\n> http-push.c | 2 +-\n> t/t5540-http-push.sh | 7 +++++++\n> 2 files changed, 8 insertions(+), 1 deletions(-)\n> \n> diff --git a/http-push.c b/http-push.c\n> index eefd64c..bd8f372 100644\n> --- a/http-push.c\n> +++ b/http-push.c\n> @@ -558,7 +558,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, strrchr(request->lock->token, ':') + 1);\n\nThis is unsafe. What if lock->token does not contain a colon? Even if it \nhappens to be the case now, in your setup, it might change, or there might \nbe mistakes in the server code. We should always play it safe if we \ncannot control the other side's code.\n\nCiao,\nDscho\n"},{"id":"103648","messageId":"7v1vuavv4l.fsf@gitster.siamese.dyndns.org","threadId":"17634","inReplyTo":"alpine.DEB.1.00.0902072114360.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] use lock token in non-URI form in start_put","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-07T20:40:10Z","receivedAt":"2009-02-07T20:40:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Sun, 8 Feb 2009, Tay Ray Chuan wrote:\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:', '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> If it is a prefix that happens to be part of the URI, but must not be used \n> by the client code as a lock token, would it not be better to store the \n> token in lock->token to begin with?\n\nLet me show more of my ignorance around this codepath.\n\nWhat is the real purpose of this appending? My understanding is that it\nis to ensure that a tentative PUT goes to a new file, so that a DAV server\nwhose PUT is not atomic (i.e. can leave a half-written bogosity when the\noperation fails for whatever reason) won't leave a broken object in its\nobject store. We MOVE it to its final destination and expect that to be\natomic.\n\nDoes the server side guarantee that the lock token string is unique in the\nsense that it does not reuse a token that was used for a recent\ntransaction that was aborted? If there is no such guarantee, then using\n(a part of) the lock token as the string we append is no better than using\na random string we choose ourselves.\n\nWe may need to send the exact lock token back for unlocking the\ntransaction we started, but I do not think it necessarily is a good idea\nto tie that to the random string we would use for PUT-then-MOVE operation.\n\nRFC 4918 (section 6.5) explicitly states that the servers are free to use\nany URI so long as it meets the uniqueness requirements, so relying on it\nbeing any form of uuid does not sound like a good idea. It can contain\nnot just a colon but other potentially problematic characters, such as a\nslash, no?\n\nThat is why I asked in my previous question what in the codepath protects\nourselves from using problematic characters.\n"},{"id":"103678","messageId":"be6fef0d0902071725i799b70efs9e36b45dc9c06246@mail.gmail.com","threadId":"17634","inReplyTo":"alpine.DEB.1.00.0902072114360.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] use lock token in non-URI form in start_put","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-02-08T01:25:31Z","receivedAt":"2009-02-08T01:25:31Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Sun, Feb 8, 2009 at 4:20 AM, Johannes Schindelin\n wrote:\n> If it is a prefix that happens to be part of the URI, but must not be used\n> by the client code as a lock token, would it not be better to store the\n> token in lock->token to begin with?\n\nThe URI form of the lock token is suitable for all other occurrences\nlock token usage; that is, only start_put needs the non-URI form,\nwhile the rest use the URI form.\n\nThat's why I only changed how start_put uses the lock token; changing\nlock token just for the sake of start_put doesn't seem very effective\nto me.\n\n> This is unsafe. What if lock->token does not contain a colon? Even if it\n> happens to be the case now, in your setup, it might change, or there might\n> be mistakes in the server code. We should always play it safe if we\n> cannot control the other side's code.\n\nPoint noted. I'll try to think of something else.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"103679","messageId":"be6fef0d0902071745r355309c2o76622b3af8df53bc@mail.gmail.com","threadId":"17634","inReplyTo":"7v1vuavv4l.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] use lock token in non-URI form in start_put","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-02-08T01:45:52Z","receivedAt":"2009-02-08T01:45:52Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Sun, Feb 8, 2009 at 4:40 AM, Junio C Hamano wrote:\n> Let me show more of my ignorance around this codepath.\n>\n> What is the real purpose of this appending? My understanding is that it\n> is to ensure that a tentative PUT goes to a new file, so that a DAV server\n> whose PUT is not atomic (i.e. can leave a half-written bogosity when the\n> operation fails for whatever reason) won't leave a broken object in its\n> object store. We MOVE it to its final destination and expect that to be\n> atomic.\n\nThis happens to be my understanding as well.\n\n> Does the server side guarantee that the lock token string is unique in the\n> sense that it does not reuse a token that was used for a recent\n> transaction that was aborted? If there is no such guarantee, then using\n> (a part of) the lock token as the string we append is no better than using\n> a random string we choose ourselves.\n\nIn section 6.5 which you cite below, the token is unique, and we hold\nthe server's word for it.\n\n> We may need to send the exact lock token back for unlocking the\n> transaction we started, but I do not think it necessarily is a good idea\n> to tie that to the random string we would use for PUT-then-MOVE operation.\n>\n> RFC 4918 (section 6.5) explicitly states that the servers are free to use\n> any URI so long as it meets the uniqueness requirements, so relying on it\n> being any form of uuid does not sound like a good idea. It can contain\n> not just a colon but other potentially problematic characters, such as a\n> slash, no?\n>\n> That is why I asked in my previous question what in the codepath protects\n> ourselves from using problematic characters.\n>\n\nYou're right, section 6.5 doesn't enforce that lock tokens are UUIDs.\n\nAny solutions to this? Perhaps one could have a function, say,\nget_unique_remote_postfix, and the function would check for URI\nschemes which we know are safe for use in file names, namely,\n\"urn:uuid:\" and \"opaqulocktoken:\". However, if its a URI we are unsure\nof, then it would generate a random string.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"103682","messageId":"7vzlgx3bnt.fsf@gitster.siamese.dyndns.org","threadId":"17634","inReplyTo":"be6fef0d0902071745r355309c2o76622b3af8df53bc@mail.gmail.com","subject":"Re: [PATCH] use lock token in non-URI form in start_put","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-08T02:28:06Z","receivedAt":"2009-02-08T02:28:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan writes:\n\n> In section 6.5 which you cite below, the token is unique, and we hold\n> the server's word for it.\n\nAre you sure you are not reading too much into that guarantee?\n\nI somehow thought that the natural reading of the guarantee is *not* that\nthe tokens are unique over the lifetime of the server installation (iow, a\nlock token you obtained today will never be used in the future, and it is\na token that the server never has used before), but it merely guarantees\nthat there are no any two outstanding locks that share the same URI, lest\none client's unlock request breaks the wrong lock. But I may be wrong here.\n\n>> That is why I asked in my previous question what in the codepath protects\n>> ourselves from using problematic characters.\n>\n> You're right, section 6.5 doesn't enforce that lock tokens are UUIDs.\n>\n> Any solutions to this? Perhaps one could have a function, say,\n> get_unique_remote_postfix, and the function would check for URI\n> schemes which we know are safe for use in file names, namely,\n> \"urn:uuid:\" and \"opaqulocktoken:\". However, if its a URI we are unsure\n> of, then it would generate a random string.\n\nLet me show even more ignorance of mine with this codepath.\n\nWhat breaks and how, if we do not even use a random string but a fixed\nsuffix \".temp\" here? I am not suggesting we actually do that, but I'd\nlike to see how important the uniqueness is here to better understand the\nissue.\n"},{"id":"103684","messageId":"be6fef0d0902071928w1d9c5a0ej851c6ef7d61c8c5@mail.gmail.com","threadId":"17634","inReplyTo":"7vzlgx3bnt.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] use lock token in non-URI form in start_put","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-02-08T03:28:27Z","receivedAt":"2009-02-08T03:28:27Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Sun, Feb 8, 2009 at 10:28 AM, Junio C Hamano wrote:\n> I somehow thought that the natural reading of the guarantee is *not* that\n> the tokens are unique over the lifetime of the server installation (iow, a\n> lock token you obtained today will never be used in the future, and it is\n> a token that the server never has used before), but it merely guarantees\n> that there are no any two outstanding locks that share the same URI, lest\n> one client's unlock request breaks the wrong lock. But I may be wrong here.\n>\n> ...\n>\n> What breaks and how, if we do not even use a random string but a fixed\n> suffix \".temp\" here? I am not suggesting we actually do that, but I'd\n> like to see how important the uniqueness is here to better understand the\n> issue.\n\nHaving a temporary file dually ensures 1) no repository corruption 2)\nno pushing simultaneously to a object file. I think an example of a\nracy situation would help illustrate why a unique temporary file is\nneeded (pun not intended).\n\nImagine if two git clients, A and B, are trying to push to the\nrepository at the same time. Let's assume that A is our \"official\"\nimplementation of http-push, while B is a rogue, forked implementation\nof A. B is rogue because, unlike A, it doesn't care about locks on\ninfo/refs, refs/head/, etc. -- stuff that A checks for before\nbeginning pushing objects. But apart from that, A and B are similar in\nall other aspects.\n\nIt so happens that they're pushing the same object, say, X, to the\nsame repository. A starts first, and during the period of the transfer\nof object X, B starts too.\n\nIf we had used the \".temp\" as you had suggested, we would have some\nproblems due to unexpected behaviour as a result of A's and B's\nsimultaneous writing to the file X.temp.\n\nIf we had used the token instead for the temporary file name, the\nchances of this occurring is lowered, but still possible. That's\nbecause we're using a token from a lock somewhat arbitrarily. For\nexample, if A was pushing to the branch \"branch_A\", it would lock\nrefs/heads/branch_A and use that lock token, and if B was pushing to\n\"branch_B\", it would use the lock token obtained from locking\nrefs/heads/branch_B.\n\nGoing back to your first point on the uniqueness of the lock token,\none only needs a token that's unique for the locks currently in\neffect. If the server had given us a token that had been used before,\nbut isn't used by anyone else *now*, then it's ok, since we wouldn't\nbe overwriting anybody else's file.\n\nThus, a token that's unique *now* is sufficient.\n\n-- \nCheers,\nRay Chuan\n"}]}