{"action":"created","issue":{"url":"https://api.github.com/repos/antirez/redis/issues/4855","repository_url":"https://api.github.com/repos/antirez/redis","labels_url":"https://api.github.com/repos/antirez/redis/issues/4855/labels{/name}","comments_url":"https://api.github.com/repos/antirez/redis/issues/4855/comments","events_url":"https://api.github.com/repos/antirez/redis/issues/4855/events","html_url":"https://github.com/antirez/redis/pull/4855","id":316059358,"number":4855,"title":"SSL implementation on all channels","user":{"login":"madolson","id":34459052,"avatar_url":"https://avatars3.githubusercontent.com/u/34459052?v=4","gravatar_id":"","url":"https://api.github.com/users/madolson","html_url":"https://github.com/madolson","followers_url":"https://api.github.com/users/madolson/followers","following_url":"https://api.github.com/users/madolson/following{/other_user}","gists_url":"https://api.github.com/users/madolson/gists{/gist_id}","starred_url":"https://api.github.com/users/madolson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/madolson/subscriptions","organizations_url":"https://api.github.com/users/madolson/orgs","repos_url":"https://api.github.com/users/madolson/repos","events_url":"https://api.github.com/users/madolson/events{/privacy}","received_events_url":"https://api.github.com/users/madolson/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":15,"created_at":"2018-04-19T22:13:00Z","updated_at":"2018-05-10T15:18:49Z","closed_at":null,"author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/antirez/redis/pulls/4855","html_url":"https://github.com/antirez/redis/pull/4855","diff_url":"https://github.com/antirez/redis/pull/4855.diff","patch_url":"https://github.com/antirez/redis/pull/4855.patch"},"body":"See SSL_README.md for more information:\r\n\r\nmake test\r\nBUILD_SSL=yes make test\r\nBUILD_SSL=yes make test-ssl\r\n\r\npass 48/48 tests for all 3 combinations\r\n\\o/ All tests passed without errors!\r\n\r\nThe cluster tests also generally pass, redis-trib doesn't fully support ssl though which causes some failures."},"comment":{"url":"https://api.github.com/repos/antirez/redis/issues/comments/388085910","html_url":"https://github.com/antirez/redis/pull/4855#issuecomment-388085910","issue_url":"https://api.github.com/repos/antirez/redis/issues/4855","id":388085910,"user":{"login":"antirez","id":65632,"avatar_url":"https://avatars0.githubusercontent.com/u/65632?v=4","gravatar_id":"","url":"https://api.github.com/users/antirez","html_url":"https://github.com/antirez","followers_url":"https://api.github.com/users/antirez/followers","following_url":"https://api.github.com/users/antirez/following{/other_user}","gists_url":"https://api.github.com/users/antirez/gists{/gist_id}","starred_url":"https://api.github.com/users/antirez/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/antirez/subscriptions","organizations_url":"https://api.github.com/users/antirez/orgs","repos_url":"https://api.github.com/users/antirez/repos","events_url":"https://api.github.com/users/antirez/events{/privacy}","received_events_url":"https://api.github.com/users/antirez/received_events","type":"User","site_admin":false},"created_at":"2018-05-10T15:18:49Z","updated_at":"2018-05-10T15:18:49Z","author_association":"OWNER","body":"I've a question about `getSslConnectionForFd()`, basically the function asserts that the fd-to-SSL-handle table is not NULL. This in theory means that there is an hard rule that says that if SSL is enabled, at every FD corresponds an SSL handle. However in other parts of the code I then find checking for NULL:\r\n\r\n```\r\n void freeClusterLink(clusterLink *link) {\r\n     if (link->fd != -1) {\r\n         aeDeleteFileEvent(server.el, link->fd, AE_READABLE|AE_WRITABLE);\r\n+#ifdef BUILD_SSL\r\n+        if(server.ssl_config.enable_ssl == true){\r\n+            serverAssert((unsigned int)link->fd < server.ssl_config.fd_to_sslconn_size);\r\n+            if(server.ssl_config.fd_to_sslconn[link->fd] != NULL){\r\n+                cleanupSslConnectionForFd(link->fd);\r\n+            }\r\n+        }\r\n+#endif\r\n     }\r\n```\r\n\r\nSo I guess that there is the possibility that the socket is double freed or alike, and we want to avoid that it creates problems? After all calling close(fd) multiple times in the same file descriptor can be also very dangerous but is not an immediate crash.\r\n\r\nHowever in such case, isn't it better to just make `getSslConnectionForFd()` check for NULL and do nothing in that case? And maybe just assert to check that we are inside the range of the conversion table? Thanks."}}
{"actor":{"display_login":"antirez"}}