New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 600045 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

QUIC: Weird interface contract / coupling in QuicStreamFactory

Project Member Reported by rsleevi@chromium.org, Apr 2 2016

Issue description

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.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a7c6592902e80d2efb2d9f3877b8bf48756289b2

commit a7c6592902e80d2efb2d9f3877b8bf48756289b2
Author: rch <rch@chromium.org>
Date: Thu Aug 18 13:49:08 2016

Move UseNonBlockingIO from a windows-only method
on UdpClientSocket and UdpServerSocket to DatagramSocket.

BUG= 600045 

Review-Url: https://codereview.chromium.org/2250473007
Cr-Commit-Position: refs/heads/master@{#412816}

[modify] https://crrev.com/a7c6592902e80d2efb2d9f3877b8bf48756289b2/content/browser/renderer_host/p2p/socket_host_udp_unittest.cc
[modify] https://crrev.com/a7c6592902e80d2efb2d9f3877b8bf48756289b2/net/dns/address_sorter_posix_unittest.cc
[modify] https://crrev.com/a7c6592902e80d2efb2d9f3877b8bf48756289b2/net/dns/mock_mdns_socket_factory.h
[modify] https://crrev.com/a7c6592902e80d2efb2d9f3877b8bf48756289b2/net/quic/chromium/quic_stream_factory.cc
[modify] https://crrev.com/a7c6592902e80d2efb2d9f3877b8bf48756289b2/net/socket/socket_test_util.cc
[modify] https://crrev.com/a7c6592902e80d2efb2d9f3877b8bf48756289b2/net/socket/socket_test_util.h
[modify] https://crrev.com/a7c6592902e80d2efb2d9f3877b8bf48756289b2/net/udp/datagram_socket.h
[modify] https://crrev.com/a7c6592902e80d2efb2d9f3877b8bf48756289b2/net/udp/fuzzed_datagram_client_socket.cc
[modify] https://crrev.com/a7c6592902e80d2efb2d9f3877b8bf48756289b2/net/udp/fuzzed_datagram_client_socket.h
[modify] https://crrev.com/a7c6592902e80d2efb2d9f3877b8bf48756289b2/net/udp/udp_client_socket.cc
[modify] https://crrev.com/a7c6592902e80d2efb2d9f3877b8bf48756289b2/net/udp/udp_client_socket.h
[modify] https://crrev.com/a7c6592902e80d2efb2d9f3877b8bf48756289b2/net/udp/udp_server_socket.cc
[modify] https://crrev.com/a7c6592902e80d2efb2d9f3877b8bf48756289b2/net/udp/udp_server_socket.h
[modify] https://crrev.com/a7c6592902e80d2efb2d9f3877b8bf48756289b2/net/udp/udp_socket_perftest.cc
[modify] https://crrev.com/a7c6592902e80d2efb2d9f3877b8bf48756289b2/net/udp/udp_socket_unittest.cc

Comment 2 by rch@chromium.org, Aug 18 2016

Status: Fixed (was: Untriaged)

Sign in to add a comment