Closed Bug 1345511 Opened 7 years ago Closed 7 years ago

Move nICEr stun local address discovery to an IPC call to enable future sandbox restrictions on content process

Categories

(Core :: WebRTC: Networking, enhancement, P1)

Product:
Core
Component:
WebRTC: Networking
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mjf, Assigned: mjf)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Proposed ipdl file for stun addr IPC call managed by PNecko
632 bytes, text/plain
jduell.mcbugs
: feedback+
Details
Bug 1345511 - pt 1 - nICEr changes to support stun addr gathering from main process.
59 bytes, text/x-review-board-request
bwc
: review+
Details
Bug 1345511 - pt 2 - add IPC mechanism for getting stun addrs on main process.
59 bytes, text/x-review-board-request
bwc
: review+
jduell.mcbugs
: review+
Details
Bug 1345511 - pt 3 - start using IPC call for stun addrs in PCMedia.
59 bytes, text/x-review-board-request
bwc
: review+
Details 
      No description provided.
Assignee: nobody → mfroman
Rank: 19
Priority: -- → P1
Jason, I'm looking for feedback on this new IPC protocol for discovering local stun addrs.  In previous conversations with Patrick he suggested that our new code live under media/mtransport since that will be the primary (probably only) use case.  I kept it as managed by PNecko since it is a networking related IPC.  I appreciate any feedback you have on this.
Attachment #8844965 - Flags: feedback?(jduell.mcbugs)
Comment on attachment 8844965 [details]
Proposed ipdl file for stun addr IPC call managed by PNecko

>include protocol PNecko;
>
>using struct StunAddr from "mozilla/net/StunAddr.h";
>using StunAddrArray from "mozilla/net/PStunAddrsParams.h";
>
>include "mozilla/net/StunAddrMessageUtils.h";
>
>namespace mozilla {
>namespace net {
>
>async protocol PStunAddrsRequest
>{
>  manager PNecko;

As far as I know it's fine for this to be managed by PNecko.  


>parent:
>  // Note: channels are opened during construction, so no open method here:
>  // see PNecko.ipdl

Is this comment still valid or is it a vestige of cut-and-paste?


>  async GetStunAddrs(bool allowLoopback,
>                     bool tcpEnabled,
>                     bool allowLinkLocal);
>
>  async __delete__();
>
>child:
>  async OnStunAddrsAvailable(StunAddrArray addrs);
>};
>
>} // namespace net
>} // namespace mozilla
Attachment #8844965 - Flags: feedback?(jduell.mcbugs) → feedback+
Thanks, Jason.  Yes, good catch on the comment.  I've removed it.
Comment on attachment 8848206 [details]
Bug 1345511 - pt 1 - nICEr changes to support stun addr gathering from main process.

https://reviewboard.mozilla.org/r/121142/#review123120

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:700
(Diff revision 1)
> -static int nr_ice_get_local_addresses(nr_ice_ctx *ctx)
> +int nr_ice_get_local_addresses(nr_ice_ctx *ctx,
> +                               nr_local_addr* stun_addrs, int stun_addr_ct)

I think this would be easier to follow if this function were renamed to nr_ice_set_local_addresses, and make it barf if none are supplied (and maybe make it barf if ctx->local_addrs is already set?), and have it not call nr_stun_find_local_addresses.

Then, down in nr_ice_gather, we check ctx->local_addrs, and if they are not set, it calls nr_stun_find_local_addresses, and then calls nr_ice_set_local_addresses with the result.
Attachment #8848206 - Flags: review?(docfaraday)
Comment on attachment 8848207 [details]
Bug 1345511 - pt 2 - add IPC mechanism for getting stun addrs on main process.

https://reviewboard.mozilla.org/r/121144/#review123442

Pretty close, but I really think we should avoid serializing wild pointers. Let's serialize nullptr instead, just to cover our butts. Some other nits too.

::: media/mtransport/ipc/NrIceStunAddrMessageUtils.h:18
(Diff revision 2)
> +    aMsg->WriteBytes((void*)aParam.localAddr(),
> +                     aParam.SerializationBufferSize());

Let's prevent the localAddr_->addr->addr wild pointer from being serialized here somehow. Make a function that returns a copy with it nulled out or something. I don't think an extra copy is going to hurt us.

::: media/mtransport/ipc/StunAddrsRequestParent.cpp:7
(Diff revision 2)
> +#include "../runnable_utils.h"
> +#include "nsNetUtil.h"
> +
> +#include "../nricectx.h"
> +#include "../nricemediastream.h" // needed only for including nricectx.h
> +#include "../nricestunaddr.h"

Let's use mtransport/ as the prefix on these.

::: media/mtransport/ipc/StunAddrsRequestParent.cpp:29
(Diff revision 2)
> +  // InitializeGlobals is done on the main thread in the content process, so
> +  // we'll do the same here since IPC happens on main thread.
> +  NrIceCtx::InitializeGlobals(allowLoopback, tcpEnabled, allowLinkLocal);

Oof. That's gross. I think the problem here is we are handling policy on addresses in two places:

In nr_stun_get_addrs we are filtering out loopback, link local, and we are also removing duplicates. This function is running on the parent process.

In nr_ice_set_local_addresses, we are handling things like default route only, forcing specific interfaces, and prioritization. This function is running on the content process.

I think the design would be cleaner if the policy logic in nr_stun_get_addrs were moved to nr_ice_set_local_addresses. That would mean the parent process wouldn't need to be configured with these policy bits.

Of course, I still think we'd be stuck with doing global init, so we would have the logging system on the parent process.

::: media/mtransport/nricestunaddr.h:16
(Diff revision 2)
> +
> +namespace mozilla {
> +
> +class NrIceStunAddr {
> +public:
> +  NrIceStunAddr(); // needed for IPC serialization

Needed for deserialization, right?

::: media/mtransport/nricestunaddr.h:22
(Diff revision 2)
> +  nr_local_addr* localAddr() { return localAddr_; }
> +  const nr_local_addr* localAddr() const { return localAddr_; }

Return references?

::: media/mtransport/nricestunaddr.h:31
(Diff revision 2)
> +  // in media/mtransport/ipc/NrIceStunAddrMessagUtils.h
> +  size_t SerializationBufferSize() const;
> +  nsresult Deserialize(const char* buffer, size_t buffer_size);
> +
> +private:
> +  nr_local_addr* localAddr_;

Any particular reason this can't just be

nr_local_addr localAddr_;

::: media/mtransport/nricestunaddr.cpp:60
(Diff revision 2)
> +  if (buffer_size != sizeof(nr_local_addr)) {
> +    MOZ_MTLOG(ML_ERROR, "Failed trying to deserialize NrIceStunAddr, "
> +                        "input buffer length (" << buffer_size <<
> +                        ") does not match required length ("
> +                        << sizeof(nr_local_addr) << ")");
> +    return NS_ERROR_FAILURE;

A MOZ_ASSERT would probably be appropriate here?

::: media/mtransport/nricestunaddr.cpp:66
(Diff revision 2)
> +  }
> +
> +  nr_local_addr* from_addr =
> +      const_cast<nr_local_addr*>((const nr_local_addr*)buffer);
> +
> +  if (nr_local_addr_copy(localAddr_, from_addr)) {

You probably need a comment here about from_addr->addr->addr being invalid, and nr_local_addr_copy fixing it.

::: media/mtransport/nricestunaddr.cpp:69
(Diff revision 2)
> +      const_cast<nr_local_addr*>((const nr_local_addr*)buffer);
> +
> +  if (nr_local_addr_copy(localAddr_, from_addr)) {
> +    MOZ_MTLOG(ML_ERROR, "Failed trying to deserialize NrIceStunAddr, "
> +                        "could not copy nr_local_addr.");
> +    return NS_ERROR_FAILURE;

Probably should assert here too.
Attachment #8848207 - Flags: review?(docfaraday) → review-
Comment on attachment 8848206 [details]
Bug 1345511 - pt 1 - nICEr changes to support stun addr gathering from main process.

https://reviewboard.mozilla.org/r/121142/#review123454

This looks pretty good to me. I have a comment on part 2 that may change this patch though, so reserving judgement until then.

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:726
(Diff revision 2)
> +//     if (!stun_addr_ct) {
> +//       /* First, gather all the local addresses we have */
> +//       if((r=nr_stun_find_local_addresses(local_addrs,MAXADDRS,&addr_ct))) {
> +//         r_log(LOG_ICE,LOG_ERR,"ICE(%s): unable to gather local addresses, trying default route",ctx->label);
> +//       }
> +//     } else {
> +//       addr_ct = MIN(stun_addr_ct, MAXADDRS);
> +//       r_log(LOG_ICE, LOG_DEBUG, "ICE(%s): copy %d pre-fetched stun addrs", ctx->label, addr_ct);
> +//       for (i=0; i<addr_ct; ++i) {
> +//         if (r=nr_local_addr_copy(&local_addrs[i], &stun_addrs[i])) {
> +//           ABORT(r);
> +//         }
> +//       }
> +//     }

Comment needs to go.

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:817
(Diff revision 2)
> +//     if ((r=nr_ice_get_local_addresses(ctx, NULL, 0)))
> +//       ABORT(r);

Remove comment.
Attachment #8848206 - Flags: review?(docfaraday)
Comment on attachment 8848208 [details]
Bug 1345511 - pt 3 - start using IPC call for stun addrs in PCMedia.

https://reviewboard.mozilla.org/r/121146/#review123458

I'm mainly concerned about the RefPtr issue, but there are some nits too.

::: media/mtransport/nricectx.h:238
(Diff revision 2)
> +  // static GetStunAddrs for use in main process to support
> +  // future sandboxing restrictions

The word "future" here is going to be out of date, and nobody will fix it (probably). Also, s/main/parent/.

::: media/mtransport/nricectx.cpp:526
(Diff revision 2)
> +nsTArray<NrIceStunAddr>
> +NrIceCtx::GetStunAddrs()

Maybe add a /* static */ comment before the definition.

::: media/mtransport/nricectx.cpp:550
(Diff revision 2)
> +  nr_local_addr* local_addrs;
> +  local_addrs = new nr_local_addr[addrs.Length()];

Pretty sure you can use a stack array here, and avoid using the heap.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h:463
(Diff revision 2)
> +    explicit StunAddrsHandler(PeerConnectionMedia *pcm) :
> +      pcm_(pcm) {}
> +    void OnStunAddrsAvailable(
> +        const mozilla::net::NrIceStunAddrArray& addrs) override;
> +   private:
> +    PeerConnectionMedia* pcm_;

Hmm. Why are we not using a RefPtr like ProtocolProxyQueryHandler?
Attachment #8848208 - Flags: review?(docfaraday) → review-
Comment on attachment 8848207 [details]
Bug 1345511 - pt 2 - add IPC mechanism for getting stun addrs on main process.

The PNecko/Necko{Child|Parent} parts looks fine.
Attachment #8848207 - Flags: review?(jduell.mcbugs) → feedback+
Comment on attachment 8848208 [details]
Bug 1345511 - pt 3 - start using IPC call for stun addrs in PCMedia.

https://reviewboard.mozilla.org/r/121146/#review123686

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:310
(Diff revision 2)
> +                                  new StunAddrsHandler(this));
> +    mStunAddrsRequest->SendGetStunAddrs();
> +  } else {
> +    // No content process, so don't need to hold up the ice event queue
> +    // until completion of stun address discovery. We can let the
> +    // discovery of stun addresses happen normally.

"normally" suggest the other way is wrong. How about "discovery of stun addresses happen without IPC in the same process"?
Comment on attachment 8848206 [details]
Bug 1345511 - pt 1 - nICEr changes to support stun addr gathering from main process.

https://reviewboard.mozilla.org/r/121142/#review123454

> Comment needs to go.

Gah - sorry!  Too much hurrying.
Comment on attachment 8848207 [details]
Bug 1345511 - pt 2 - add IPC mechanism for getting stun addrs on main process.

https://reviewboard.mozilla.org/r/121144/#review123442

> Any particular reason this can't just be
> 
> nr_local_addr localAddr_;

Yes, because the IPC-related code doesn't have access to the actual definition of nr_local_addr.  There were so many hoops to jump through there to export the headers from down in nICEr and have it build on all the platforms (windows in particular caused much pain).  I'm open to suggestions, but this seemed like the easiest way to keep nICEr details encapsulated.
Comment on attachment 8848208 [details]
Bug 1345511 - pt 3 - start using IPC call for stun addrs in PCMedia.

https://reviewboard.mozilla.org/r/121146/#review123458

> Pretty sure you can use a stack array here, and avoid using the heap.

Windows builds complained on try when I attempted that before.
Comment on attachment 8848206 [details]
Bug 1345511 - pt 1 - nICEr changes to support stun addr gathering from main process.

https://reviewboard.mozilla.org/r/121142/#review124680
Attachment #8848206 - Flags: review?(docfaraday) → review+
Comment on attachment 8848207 [details]
Bug 1345511 - pt 2 - add IPC mechanism for getting stun addrs on main process.

https://reviewboard.mozilla.org/r/121144/#review124686

::: media/mtransport/ipc/NrIceStunAddrMessageUtils.h:19
(Diff revisions 2 - 3)
>  struct ParamTraits<mozilla::NrIceStunAddr>
>  {
>    static void Write(Message* aMsg, const mozilla::NrIceStunAddr &aParam)
>    {
> -    aMsg->WriteBytes((void*)aParam.localAddr(),
> -                     aParam.SerializationBufferSize());
> +    const size_t bufSize = aParam.SerializationBufferSize();
> +    char* buffer = new char[bufSize];

We need to delete[] after, right?
Attachment #8848207 - Flags: review?(docfaraday) → review+
Comment on attachment 8848208 [details]
Bug 1345511 - pt 3 - start using IPC call for stun addrs in PCMedia.

https://reviewboard.mozilla.org/r/121146/#review124690
Attachment #8848208 - Flags: review?(docfaraday) → review+
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e0a12ec4de38
pt 1 - nICEr changes to support stun addr gathering from main process. r=bwc
https://hg.mozilla.org/integration/autoland/rev/af9901df3d48
pt 2 - add IPC mechanism for getting stun addrs on main process. r=bwc
https://hg.mozilla.org/integration/autoland/rev/6fe853638e4d
pt 3 - start using IPC call for stun addrs in PCMedia. r=bwc
Keywords: checkin-needed
Depends on: 1350055
Attachment #8848207 - Flags: review?(jduell.mcbugs) → review+
Depends on: 1350568
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: