{"action":"created","issue":{"url":"https://api.github.com/repos/apache/guacamole-server/issues/179","repository_url":"https://api.github.com/repos/apache/guacamole-server","labels_url":"https://api.github.com/repos/apache/guacamole-server/issues/179/labels{/name}","comments_url":"https://api.github.com/repos/apache/guacamole-server/issues/179/comments","events_url":"https://api.github.com/repos/apache/guacamole-server/issues/179/events","html_url":"https://github.com/apache/guacamole-server/pull/179","id":349161840,"node_id":"MDExOlB1bGxSZXF1ZXN0MjA3MzQzMDQ4","number":179,"title":"GUACAMOLE-608: Fix connection write thread may encounter dead loop","user":{"login":"changkun","id":5498964,"node_id":"MDQ6VXNlcjU0OTg5NjQ=","avatar_url":"https://avatars3.githubusercontent.com/u/5498964?v=4","gravatar_id":"","url":"https://api.github.com/users/changkun","html_url":"https://github.com/changkun","followers_url":"https://api.github.com/users/changkun/followers","following_url":"https://api.github.com/users/changkun/following{/other_user}","gists_url":"https://api.github.com/users/changkun/gists{/gist_id}","starred_url":"https://api.github.com/users/changkun/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/changkun/subscriptions","organizations_url":"https://api.github.com/users/changkun/orgs","repos_url":"https://api.github.com/users/changkun/repos","events_url":"https://api.github.com/users/changkun/events{/privacy}","received_events_url":"https://api.github.com/users/changkun/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":15,"created_at":"2018-08-09T14:48:59Z","updated_at":"2018-09-18T19:15:48Z","closed_at":"2018-08-23T07:32:55Z","author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/apache/guacamole-server/pulls/179","html_url":"https://github.com/apache/guacamole-server/pull/179","diff_url":"https://github.com/apache/guacamole-server/pull/179.diff","patch_url":"https://github.com/apache/guacamole-server/pull/179.patch"},"body":"guacd involves a `__write_all` function to write instruction as much as possible. However, system call `write` is possible [return 0 and also set errno](https://linux.die.net/man/2/write), which is not verified in the function.\r\n\r\nA possible case is `write` keeps return 0 (nothing writes to buffer). Therefore the daemon process encounters a dead loop. Furthermore, it leads CPU rate up to 99%.\r\n\r\nThe above phenomenon is observed from server:\r\n\r\n```bash\r\n$ top\r\n  PID USER      PR  NI    VIRT    RES    SHR S  %CPU %MEM     TIME+ COMMAND\r\n15176 root      20   0 1988580  25720   3168 S  99.9  0.3  12176:50 guacd\r\n25414 root      20   0 1726048  23324   3040 S  99.9  0.3  17227:15 guacd\r\n...\r\n\r\n$ sudo lsof -i | grep 15176 # no network connection\r\n$ top -H -p 25414\r\n  PID USER      PR  NI    VIRT    RES    SHR S %CPU %MEM     TIME+ COMMAND\r\n25418 root      20   0 1726048  23324   3040 R 99.9  0.3  17230:06 guacd\r\n25414 root      20   0 1726048  23324   3040 S  0.0  0.3   0:00.00 guacd\r\n25420 root      20   0 1726048  23324   3040 S  0.0  0.3   0:06.16 guacd\r\n```\r\n\r\nThis patch perhaps needs more discussion on when and why this is happening, as well as whether should we record the failure to log, for instance:\r\n\r\n```c\r\nint written = write(fd, buffer, length);\r\nif (written < 0)\r\n    return -1;\r\nif (written == 0 && errno > 0) {\r\n    guacd_log(GUAC_LOG_ERROR, \"Unable to write buffer to file descriptor: %s\", strerror(errno));\r\n    return -1;\r\n}\r\n```"},"comment":{"url":"https://api.github.com/repos/apache/guacamole-server/issues/comments/422514340","html_url":"https://github.com/apache/guacamole-server/pull/179#issuecomment-422514340","issue_url":"https://api.github.com/repos/apache/guacamole-server/issues/179","id":422514340,"node_id":"MDEyOklzc3VlQ29tbWVudDQyMjUxNDM0MA==","user":{"login":"changkun","id":5498964,"node_id":"MDQ6VXNlcjU0OTg5NjQ=","avatar_url":"https://avatars3.githubusercontent.com/u/5498964?v=4","gravatar_id":"","url":"https://api.github.com/users/changkun","html_url":"https://github.com/changkun","followers_url":"https://api.github.com/users/changkun/followers","following_url":"https://api.github.com/users/changkun/following{/other_user}","gists_url":"https://api.github.com/users/changkun/gists{/gist_id}","starred_url":"https://api.github.com/users/changkun/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/changkun/subscriptions","organizations_url":"https://api.github.com/users/changkun/orgs","repos_url":"https://api.github.com/users/changkun/repos","events_url":"https://api.github.com/users/changkun/events{/privacy}","received_events_url":"https://api.github.com/users/changkun/received_events","type":"User","site_admin":false},"created_at":"2018-09-18T19:15:48Z","updated_at":"2018-09-18T19:15:48Z","author_association":"NONE","body":"FYI: the write call is called correctly in guacd. However check errno in this PR is still a right way.\r\n\r\nI quote an answer from Dietrich on [StackOverflow](https://stackoverflow.com/questions/52081841/differences-between-c-write-call-and-go-syscall-write) in below, and \r\n\r\nTLDR: **errno is not set if write return zero, but it can be negative is it was not reset by caller.**\r\n\r\n> With write, there are only two cases to consider:\r\n> \r\n> - If it fails, the result is -1 and errno is set.\r\n> - If it succeeds, the result is 0 or larger and errno is not set.\r\n> There are no other cases to consider, unless you are interested in historical Unix implementations (see: [Is a return value of 0 from write(2) in C an error?](https://stackoverflow.com/questions/2176443/is-a-return-value-of-0-from-write2-in-c-an-error)).\r\n> \r\n> The reason that write may return 0 is because the input buffer may be empty.\r\n> \r\n> **Q**: However, the man page of C write call roughly describes errno may also be set but unspecified if we write zero length buffer without explaining any detail.\r\n> \r\n> **A**: All this means is that it's possible for a 0-length write to fail. If it fails, it returns -1 and sets errno. If it succeeds, it returns 0 and does not set errno. This is the same behavior for any other write, it's just mentioned in the man page because people may find it surprising that a 0-length write could fail.\r\n> \r\n> **Q**: What is the status of errno if write call returning 0 for a file, a non-blocking socket, or a blocking socket?\r\n> \r\n> In this case, errno is not set, because write did not fail. This will only happen if the input buffer is zero bytes.\r\n> \r\n> **Q**: When and how write call returning 0 and errno is not 0?\r\n> \r\n> **A**: This does not happen. Either errno is set and the return is -1, or errno is not set and the return is 0 or larger.\r\n> \r\n> **Q**: What is the status of errno if write call returning positive? Will it be negative?\r\n> \r\n> **A**: The errno value will not be set. It will have the same value as it did before the write call.\r\n> \r\n> **Q**: Is there any other syscall may encounter the same situation?\r\n> \r\n> In general, system calls will either return an error or they will succeed. They won't do some mixture of both. Look at the Return Value section of other man pages and you will see that they are mostly the same as write.\r\n\r\n"}}
{"actor":{"display_login":"changkun"}}