Avoid static_cast<SSLClientSocket>(StreamSocket) |
|||
Issue descriptionSSLClientSocket is a derived class of SSLSocket, which in turn is a derived class of StreamSocket. Some TLS-related methods, like WasAlpnNegotiated(), GetNegotiatedProtocol(), and GetSSLInfo(), are virtual methods of StreamSocket, even though many implementations return false, an error, or crash with NOTREACHED(). Some other TLS-related methods, most notably GetSSLCertRequestInfo() and GetTokenBindingSignature(), are methods of SSLClientSocket, which makes sense (these methods should not be called on any other StreamSocket subclass). However, this results in a StreamSocket (the return type of ClientSocketHandle::socket()) having to be cast to an SSLClientSocket at at least five call sites: https://cs.chromium.org/chromium/src/net/http/http_stream_parser.cc?q=GetSSLCertRequestInfo&l=1137 https://cs.chromium.org/chromium/src/net/http/http_stream_parser.cc?q=GetTokenBindingSignature&l=1150 https://cs.chromium.org/chromium/src/net/spdy/chromium/spdy_session.cc?q=GetTokenBindingSignature&l=1196 The other two being checked in at https://crrev.com/c/980964. One reviewer of https://crrev.com/c/980964 has asked for a comment to explain why this cast is safe, rightfully so. The other three sites do not have such comments. Also, there must be a better way of avoiding undefined behavior in the long run. I propose to move at least these two methods (GetSSLCertRequestInfo and GetTokenBindingSignature), if not all non-static public SSLClientSocket methods, to StreamSocket, and adding a dummy implementation to all other derived classes. Also, in the future, potentially other classes derived from StreamSocket, like HttpProxyClientSocket, HttpProxyClientSocketWrapper, QuicProxyClientSocket, SOCKSClientSocket, and SOCKS5ClientSocket, might need to have a meaningful implementation for GetSSLCertRequestInfo() or GetTokenBindingSignature(), just like they all already have one for GetSSLInfo(), even though they are not SSLSocket subclasses.
,
Mar 28 2018
It could be reasonable to have GetTokenBindingSignature return ERR_UNIMPLEMENTED or call NOTREACHED() in the cases where it doesn't make sense. The casts that exist so far don't seem unreasonable to me, although maybe it's better to hit a NOTREACHED() in a dummy implementation than to hit undefined behavior in a bad cast (if such a thing ever happens).
,
Mar 28 2018
Agreed. I think at some point I considered going the other direction (leaning on the cast more) and mmenke@ and rsleevi@ said they instead preferred putting it in StreamSocket. The one thing I would suggest is that we probably should provide a default implementation. Adding a new StreamSocket is a rather unreasonable amount of boilerplate, and it's not like anything other than SSLClientSocket would be implementing these. Asking everyone to implement it would even make them likely to forward to some underlying socket, which would be incorrect. E.g. an HTTP CONNECT proxy socket over an HTTPS proxy should *not* forward ALPN or exporters from the underlying on. (I think I came across some instances of folks forwarding ALPN that I was meaning to clear through and never did...)
,
Apr 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dae8af5fe6c8d1c26f489fa103bc51427d72678b commit dae8af5fe6c8d1c26f489fa103bc51427d72678b Author: Bence Béky <bnc@chromium.org> Date: Fri Apr 13 08:53:17 2018 Avoid static_cast<SSLClientSocket>(StreamSocket). Move four methods: GetSSLCertRequestInfo(), GetChannelIDService(), GetTokenBindingSignature(), and GetChannelIDKey() from SSLClientSocket to its base class's base class, StreamSocket. Also make GetSSLCertRequestInfo() const. Provide dummy implementations, with NOTREACHED(), in base class, to make it less painful to implement a new derived class. Remove 11 accounts of static_cast<SSLClientSocket*>(StreamSocket). The only remaining invocations are within SSLClientSocketImpl. Bug: 826713 Change-Id: I1f87d31204b95687e6552b7d6f02578de4321eaf Reviewed-on: https://chromium-review.googlesource.com/1010106 Commit-Queue: Bence Béky <bnc@chromium.org> Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org> Reviewed-by: Nick Harper <nharper@chromium.org> Cr-Commit-Position: refs/heads/master@{#550554} [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/chrome/browser/extensions/api/socket/tls_socket_unittest.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/BUILD.gn [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/http/http_proxy_client_socket_wrapper.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/http/http_stream_factory_impl_job.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/http/http_stream_parser.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/socket/fuzzed_socket_factory.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/socket/socket_test_util.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/socket/socket_test_util.h [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/socket/ssl_client_socket.h [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/socket/ssl_client_socket_impl.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/socket/ssl_client_socket_impl.h [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/socket/ssl_client_socket_pool_unittest.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/socket/stream_socket.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/socket/stream_socket.h [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/spdy/chromium/spdy_session.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/websockets/websocket_basic_handshake_stream.cc
,
Apr 13 2018
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dae8af5fe6c8d1c26f489fa103bc51427d72678b commit dae8af5fe6c8d1c26f489fa103bc51427d72678b Author: Bence Béky <bnc@chromium.org> Date: Fri Apr 13 08:53:17 2018 Avoid static_cast<SSLClientSocket>(StreamSocket). Move four methods: GetSSLCertRequestInfo(), GetChannelIDService(), GetTokenBindingSignature(), and GetChannelIDKey() from SSLClientSocket to its base class's base class, StreamSocket. Also make GetSSLCertRequestInfo() const. Provide dummy implementations, with NOTREACHED(), in base class, to make it less painful to implement a new derived class. Remove 11 accounts of static_cast<SSLClientSocket*>(StreamSocket). The only remaining invocations are within SSLClientSocketImpl. Bug: 826713 Change-Id: I1f87d31204b95687e6552b7d6f02578de4321eaf Reviewed-on: https://chromium-review.googlesource.com/1010106 Commit-Queue: Bence Béky <bnc@chromium.org> Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org> Reviewed-by: Nick Harper <nharper@chromium.org> Cr-Commit-Position: refs/heads/master@{#550554} [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/chrome/browser/extensions/api/socket/tls_socket_unittest.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/BUILD.gn [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/http/http_proxy_client_socket_wrapper.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/http/http_stream_factory_impl_job.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/http/http_stream_parser.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/socket/fuzzed_socket_factory.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/socket/socket_test_util.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/socket/socket_test_util.h [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/socket/ssl_client_socket.h [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/socket/ssl_client_socket_impl.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/socket/ssl_client_socket_impl.h [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/socket/ssl_client_socket_pool_unittest.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/socket/stream_socket.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/socket/stream_socket.h [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/spdy/chromium/spdy_session.cc [modify] https://crrev.com/dae8af5fe6c8d1c26f489fa103bc51427d72678b/net/websockets/websocket_basic_handshake_stream.cc |
|||
►
Sign in to add a comment |
|||
Comment 1 by b...@chromium.org
, Mar 28 2018