Closed
Bug 1304568
Opened 8 years ago
Closed 8 years ago
A peculiar class hierarchy with NrSocketBase as a root
Categories
(Core :: WebRTC: Networking, defect, P2)
Core
WebRTC: Networking
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
Updated•8 years ago
|
Component: Build Config → WebRTC: Networking
Product: Firefox → Core
Reporter | ||
Comment 1•8 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•8 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•8 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)
Updated•8 years ago
|
Rank: 25
Priority: -- → P2
Comment hidden (mozreview-request) |
Comment 5•8 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•8 years ago
|
Flags: needinfo?(enes.goktas)
Assignee | ||
Comment 6•8 years ago
|
||
Does the code in the review push work for you?
Flags: needinfo?(enes.goktas)
Comment 7•8 years ago
|
||
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.
Comment 8•8 years ago
|
||
Here is one more: http://searchfox.org/mozilla-central/source/media/mtransport/test_nr_socket.cpp#240
Comment 9•8 years ago
|
||
Not sure about this one: http://searchfox.org/mozilla-central/source/media/mtransport/test_nr_socket.cpp#653
Comment 10•8 years ago
|
||
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•8 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.
Comment 12•8 years ago
|
||
(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•8 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 14•8 years ago
|
||
Pushed by bcampen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8de3dbbe8e3a Fix some bad casts. r=drno
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8de3dbbe8e3a
Status: NEW → RESOLVED
Closed: 8 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.
Description
•