Closed Bug 1304568 Opened 3 years ago Closed 3 years ago

A peculiar class hierarchy with NrSocketBase as a root

Categories

(Core :: WebRTC: Networking, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: egoktas, Assigned: bwc)

Details

Attachments

(1 file)

During the process of making Firefox compatible with CFI in Clang, we noticed an interesting behavior: 
- TestNrSocket, NrUdpSocketIpc, NrTcpSocketIpc objects are used at locations in code where an object of type NrSocket is expected, although these types do not inherit from each other

E.g.: http://searchfox.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#2258
---------------------------------------------------------------------------
static int nr_socket_local_connect(void *obj, nr_transport_addr *addr) {
  NrSocket *sock = static_cast<NrSocket *>(obj);

  return sock->connect(addr); // <- NrSocket expected, but classes that are 
                              //     not descendant of NrSocket are used.
}
---------------------------------------------------------------------------

The security checks of CFI hinders and breaks this behavior, e.g. it disallows an object of NrUdpSocketIpc to be used at a location in code where NrSocket is used as a type. CFI would only allow objects of type NrSocket and NrSocketBase at these locations (see class hierarchy below).

We are wondering whether the structure of the class hierarchy and the behavior is done as such on purpose, and whether we can restructure it such that the classes inherit from NrSocket. Restructuring can get rid of the explicit type casts. In terms of the CFI security checks, objects of NrSocket's descendants (TestNrSocket, NrUdpSocketIpc, NrTcpSocketIpc) will be allowed at locations in code where the NrSocket type is used.

Instead of restructuring, another option could be to replace NrSocket with NrSocketBase in the code. This can also get rid of the explicit casts, because all of the nr socket classes inherit from the NrSocketBase class.

The class hierarchy with NrSocketBase as the root looks as follows:

       NrSocketBase
        /   | \    \
       /    |  \   \/
      /     |   \  DummySocket   
     \/     |    \      
  NrSocket  |    \/     
            |   TestNrSOcket 
           \./   
       NrSocketIpc
          /    \
         /      \
        /        \
       /          \
      \/          \/
NrUdpSocketIpc   NrTcpSocketIpc
Component: Build Config → WebRTC: Networking
Product: Firefox → Core
Errata in description:
".. CFI would only allow objects of type NrSocket and NrSocketBase at these locations .."
=>
".. CFI would only allow objects of type NrSocket at these locations .."

CFI would not allow objects of the type NrSocketBase at locations where the type is NrSocket. To allow NrSocketBase, NrSocketBase had to be a descendant of NrSocket.
Yeah, that's pretty gross. I'm pretty sure the answer is to use NrSocketBase everywhere this occurs. I'll give that a whirl today.
Assignee: nobody → docfaraday
I've fixed the bad casts in nr_socket_prsock.cpp, and don't see others elsewhere. Are there any other errors I should be fixing?
Flags: needinfo?(enes.goktas)
Rank: 25
Priority: -- → P2
Comment on attachment 8794296 [details]
Bug 1304568: Fix some bad casts.

https://reviewboard.mozilla.org/r/80810/#review80022

LGTM
Attachment #8794296 - Flags: review?(drno) → review+
Flags: needinfo?(enes.goktas)
Does the code in the review push work for you?
Flags: needinfo?(enes.goktas)
I think this: 
http://searchfox.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#2139
and the comment here:
http://searchfox.org/mozilla-central/source/media/mtransport/nr_socket_prsock.h#99
should be fixed as well. All the callers of CreateSocket() I could find expect a NrSocketBase and not just a NrSocket.
If we remove the few remaining occurrences I think we could remove NrSocket all together, because there is no usage left as far as I can tell.
(In reply to Nils Ohlmeier [:drno] from comment #7)
> I think this: 
> http://searchfox.org/mozilla-central/source/media/mtransport/
> nr_socket_prsock.cpp#2139
> and the comment here:
> http://searchfox.org/mozilla-central/source/media/mtransport/
> nr_socket_prsock.h#99
> should be fixed as well. All the callers of CreateSocket() I could find
> expect a NrSocketBase and not just a NrSocket.

An NrSocket is an NrSocketBase, so these should be fine. I can fix the log strings in comment 8 and comment 9.
(In reply to Byron Campen [:bwc] from comment #11)
> (In reply to Nils Ohlmeier [:drno] from comment #7)
> > I think this: 
> > http://searchfox.org/mozilla-central/source/media/mtransport/
> > nr_socket_prsock.cpp#2139
> > and the comment here:
> > http://searchfox.org/mozilla-central/source/media/mtransport/
> > nr_socket_prsock.h#99
> > should be fixed as well. All the callers of CreateSocket() I could find
> > expect a NrSocketBase and not just a NrSocket.
> 
> An NrSocket is an NrSocketBase, so these should be fine.

Okay. I think it's just the very last occurrence of NrSocket. So if we remove that we could eliminate NrSocket in any way we want.
(In reply to Byron Campen [:bwc] from comment #3)
> I've fixed the bad casts in nr_socket_prsock.cpp, and don't see others
> elsewhere. Are there any other errors I should be fixing?

Applying your NrSocket -> NrSocketBase patch fixed the error. There are currently no other errors related to NrSocket from my side.
Flags: needinfo?(enes.goktas)
https://hg.mozilla.org/mozilla-central/rev/8de3dbbe8e3a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.