{"action":"created","issue":{"url":"https://api.github.com/repos/fkooman/php-remote-storage/issues/40","labels_url":"https://api.github.com/repos/fkooman/php-remote-storage/issues/40/labels{/name}","comments_url":"https://api.github.com/repos/fkooman/php-remote-storage/issues/40/comments","events_url":"https://api.github.com/repos/fkooman/php-remote-storage/issues/40/events","html_url":"https://github.com/fkooman/php-remote-storage/issues/40","id":126063472,"number":40,"title":"Race conditions in PUT leading to file corruption","user":{"login":"untitaker","id":837573,"avatar_url":"https://avatars.githubusercontent.com/u/837573?v=3","gravatar_id":"","url":"https://api.github.com/users/untitaker","html_url":"https://github.com/untitaker","followers_url":"https://api.github.com/users/untitaker/followers","following_url":"https://api.github.com/users/untitaker/following{/other_user}","gists_url":"https://api.github.com/users/untitaker/gists{/gist_id}","starred_url":"https://api.github.com/users/untitaker/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/untitaker/subscriptions","organizations_url":"https://api.github.com/users/untitaker/orgs","repos_url":"https://api.github.com/users/untitaker/repos","events_url":"https://api.github.com/users/untitaker/events{/privacy}","received_events_url":"https://api.github.com/users/untitaker/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":4,"created_at":"2016-01-11T23:37:15Z","updated_at":"2016-01-12T12:03:54Z","closed_at":null,"body":"## Example\r\n\r\nTwo PUTs (called `a` and `b`) requests to the same filepath with `If-None-Match: *` are launched at the same time:\r\n\r\n*  `RemoteStorage->putDocument` is called for both requests. Precondition checking succeeds in both cases because the file does not exist yet, and in each request process/thread, the function proceeds to call [`DocumentStorage->putDocument`](https://github.com/fkooman/php-remote-storage/blob/569b0ee189ebd8ee0831f2e436188b0d5f2ae9e2/src/fkooman/RemoteStorage/DocumentStorage.php#L82).\r\n* The routine for creating the necessary subfolders (the loop at the start of `DocumentStorage->putDocument`) may or may not happen in parallel. If it does happen in parallel, the upload will crash due to another (unimportant) race condition. But if it does not crash...\r\n* `file_put_contents` is called for request `a`. Since the file doesn't exist, it is created. A filepointer to that file is opened, and the content of request `a` is written to it.\r\n* `file_put_contents` is called for request `b`. Since the file does exist, a filepointer to it is opened. The content of request `b` is written to it.\r\n* Any not-trivially-small file will require multiple `write`s to the same filepointer. The writes for both requests may become interleaved in arbitrary ways. Some writes may overwrite others. The resulting file becomes garbage. `201 Created`. The end.\r\n\r\n----\r\n\r\nThere are more race conditions in php-remote-storage, and there are more scenarios with the same race condition as root cause.\r\n\r\n## Solution\r\n\r\nLocks everywhere. Basically lock the whole user when creating directories, and later only lock the target file. You need the same thing for GET, DELETE, etc.\r\n\r\nI'm not very familiar with PHP, but I'd advise not to use builtin `flock`. It uses POSIX advisory locking, which (at least on Unix) means that your file lock is per-process, which means that one process can call `flock` repeatedly without waiting. This is totally fine if each request has its own process, but it renders the whole mechanism completely useless otherwise (every request is able to acquire the lock at the same time, since it's all in the same process). In short, whether `flock` actually works depends on what appserver you use."},"comment":{"url":"https://api.github.com/repos/fkooman/php-remote-storage/issues/comments/170891985","html_url":"https://github.com/fkooman/php-remote-storage/issues/40#issuecomment-170891985","issue_url":"https://api.github.com/repos/fkooman/php-remote-storage/issues/40","id":170891985,"user":{"login":"untitaker","id":837573,"avatar_url":"https://avatars.githubusercontent.com/u/837573?v=3","gravatar_id":"","url":"https://api.github.com/users/untitaker","html_url":"https://github.com/untitaker","followers_url":"https://api.github.com/users/untitaker/followers","following_url":"https://api.github.com/users/untitaker/following{/other_user}","gists_url":"https://api.github.com/users/untitaker/gists{/gist_id}","starred_url":"https://api.github.com/users/untitaker/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/untitaker/subscriptions","organizations_url":"https://api.github.com/users/untitaker/orgs","repos_url":"https://api.github.com/users/untitaker/repos","events_url":"https://api.github.com/users/untitaker/events{/privacy}","received_events_url":"https://api.github.com/users/untitaker/received_events","type":"User","site_admin":false},"created_at":"2016-01-12T12:03:54Z","updated_at":"2016-01-12T12:03:54Z","body":"Are you sure A actually opens the connection first? Perhaps B finishes before A opens the connection. Are you sending \"If-None-Match: *\"?\n\nThat header is important, otherwise that behavior would be perfectly fine.\n\nOn 12 January 2016 11:59:35 CET, \"François Kooman\" <notifications@github.com> wrote:\n>I just tested this on storage.tuxed.net. Process A is a 1MB upload,\n>process B is a 1kB upload. \n>\n>I start A, and immediately after start B. A keeps going, B finishes\n>first, a while after A completes as well. The actual file written is\n>the contents of A. I checked with a sha256sum, which is kind\n>interesting. Maybe it is by accident?\n>\n>https://github.com/fkooman/php-remote-storage/blob/569b0ee189ebd8ee0831f2e436188b0d5f2ae9e2/src/fkooman/RemoteStorage/DocumentStorage.php#L103\n>\n>I could enable locking for `file_put_contents`, see\n>https://secure.php.net/manual/en/function.file-put-contents.php, but\n>this will not be quite enough as we want to terminate the second\n>attempt with an error instead of just waiting for the first to succeed\n>and then overwrite the first one again, or not? The specification says\n>nothing about the intended behavior...maybe we should make that\n>explicit? What do you think? How does your server respond?\n>\n>---\n>Reply to this email directly or view it on GitHub:\n>https://github.com/fkooman/php-remote-storage/issues/40#issuecomment-170876121\n\n-- \nSent from my phone. Please excuse my brevity.\n"}}