A peculiar class hierarchy with NrSocketBase as a root

RESOLVED FIXED in Firefox 52

Status

()

Core
WebRTC: Networking
P2
normal
Rank:
25
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: egoktas, Assigned: bwc)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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
(Reporter)

Comment 1

2 years ago
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.
(Assignee)

Comment 2

2 years ago
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
(Assignee)

Comment 3

2 years ago
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 hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
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+
(Assignee)

Updated

2 years ago
Flags: needinfo?(enes.goktas)
(Assignee)

Comment 6

2 years ago
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.
(Assignee)

Comment 11

2 years ago
(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.
(Reporter)

Comment 13

2 years ago
(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)

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8de3dbbe8e3a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.