While trying to read and understand the relationship between //net/quic and //net/socket in the dependency graph, I noticed what I would term as a weird interface contract, because it represents a dangerous (has caused real security bugs) pattern.
In this case, it's QuicStreamFactory testing whether or not client_socket_factory_ is equivalent to the ClientSocketFactory::GetDefaultFactory(), and if it is, static_cast<>ing it as a UDPClientSocket.
This seems like a code smell/tight coupling in that:
- Assumes that GetDefaultFactory will always return the same pointer (not part of the API contract)
- Assumes that GetDefaultFactory will always return a UDPClientSocket as part of CreateDatagramClientSocket (not guaranteed by the API contract; it returns a DatagramClientSocket, a base class of UDPClientSocket)
It seems like this sort of layering / casting should be done by moving the non-blocking IO configuration to the DatagramClientSocket (or its bases, DatagramSocket / Socket), rather than to the UdpClientSocket.
It also feels like inventing an RTTI-like workaround ( https://google.github.io/styleguide/cppguide.html#Run-Time_Type_Information__RTTI_ ), in that you're effectively abstracting the sub-classes' type into the value of the factory pointer.
Because I don't use UDPSocket enough, I'm not sure what of the possible solutions is correct, but the code as written seems error prone and may cause security bugs, much like the liberal casting to sub-classes in our proxy logic did.
Comment 1 by bugdroid1@chromium.org
, Aug 18 2016