Closed Bug 1326483 Opened 7 years ago Closed 7 years ago

UDP socket child allows network connectivity in offline mode

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: drno, Assigned: schien)

Details

(Whiteboard: [necko-active])

Attachments

(2 files)

Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.
59 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1326483 - Part 2, test cases.
59 bytes, text/x-review-board-request
drno
: review+
Details 
Currently WebRTC's PeerConnection.js monitors and clones the network online/offline state to prevent the usage of WebRTC in offline mode.

I think the parent process should return errors to the UDP socket child implementation in case it tries to access to network (anything else then loopback).

Note: currently WebRTC's UDP socket child implementation can create and use network sockets even when Fx is in the offline mode.
Component: Networking → WebRTC: Networking
@michal: sorry this is not a WebRTC Networking bug, as this the problem here is not in WebRTC's networking code.

I reported this as in Networking as I think this should be fixed in the UDPSocketParent class, which according to some quick blame annotations has been used before to implement the UDPSocketParent class.

The WebRTC Networking folks can certainly help fixing this, e.g. by providing patches, once we have consensus from the Necko folks where to fix this.
Component: WebRTC: Networking → Networking
Assignee: nobody → schien
Whiteboard: [necko-active]
makes sense to me nils. thanks for noting the localhost bit
I'll recommend to add offline mode check in nsUDPSocket since there are usages that directly access nsUDPSocket without going through IPC.
There are 5 users of nsUDPSocket in mozilla-central.

1. WebRTC: use via UDPSocket Parent, already handle the offline mode scenario.
2. UDPSocket WebAPI: use via UDPSocketParent, will throw NetworkError to content JS.
3. SimpleServiceDiscovery.jsm: use directly, have a retry timer if failed to create socket.
4. MulticastDNS.jsm: use directly, doesn't handle socket creation failure
5. DevTool: use directly, socket creation failure is caught but do nothing.

I'll file follow-up bugs for MulticastDNS.jsm and DevTool to notify them adding error handling.
File bug 1328504 for DevTool and bug 1328506 for MulticastDNS.jsm according to comment #6.
Comment on attachment 8823495 [details]
Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.

https://reviewboard.mozilla.org/r/102020/#review102400

I think the CheckIOStatus() call should also be made from the send() function, because WebRTC never connects it's UDP sockets. It leaves them unconnected and calls sendto() on the socket to send to different destinations.
Comment on attachment 8823495 [details]
Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.

https://reviewboard.mozilla.org/r/102020/#review102400

IIRC, UDP socket will be detached once entering offline. Therefore, either checking attached state or calling `CheckIOStatus()` will be enough.
Attachment #8823495 - Flags: review?(mcmanus)
Comment on attachment 8823495 [details]
Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.

https://reviewboard.mozilla.org/r/102020/#review102574
Attachment #8823495 - Flags: review?(mcmanus)
Attachment #8823495 - Flags: review?(mcmanus) → review?(drno)
Comment on attachment 8823495 [details]
Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.

https://reviewboard.mozilla.org/r/102020/#review102734

LGTM.
Manually verified that WebRTC no longer can make calls in offline mode. But it works for loopback with the fix mentioned below.

::: netwerk/base/nsUDPSocket.cpp:618
(Diff revision 2)
>  {
>    NS_ENSURE_TRUE(mFD == nullptr, NS_ERROR_ALREADY_INITIALIZED);
>  
> -  if (gIOService->IsNetTearingDown()) {
> -    return NS_ERROR_FAILURE;
> +  nsresult rv;
> +
> +  rv = CheckIOStatus(&mAddr);

You need to use |aAddr| here instead of |mAddr|.
Attachment #8823495 - Flags: review?(drno) → review+
Comment on attachment 8823495 [details]
Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.

you guys make a good team for udpsocket - keep it up :)
Attachment #8823495 - Flags: review?(mcmanus) → review?(drno)
Attachment #8823962 - Flags: review?(mcmanus) → review?(drno)
Comment on attachment 8823495 [details]
Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.

https://reviewboard.mozilla.org/r/102020/#review103132
Attachment #8823495 - Flags: review?(drno) → review+
Comment on attachment 8823962 [details]
Bug 1326483 - Part 2, test cases.

https://reviewboard.mozilla.org/r/102440/#review103268
Attachment #8823962 - Flags: review?(drno) → review+
Hmm...test_udpsocket_offline.js is failed on Win 7 because fail to bind socket to ipv6 loopback address '::1'.
>WARNING: failed to bind socket: file c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/netwerk/base/nsUDPSocket.cpp, line 661
Any idea?
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #20)
> Maybe related to bug 892019?

Quite possible. In all of our WebRTC tests we always check if IPv6 is available and skip tests if it's missing, because otherwise too many test fail :-(
(In reply to Nils Ohlmeier [:drno] from comment #21)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #20)
> > Maybe related to bug 892019?
> 
> Quite possible. In all of our WebRTC tests we always check if IPv6 is
> available and skip tests if it's missing, because otherwise too many test
> fail :-(

I just figured out the issue on Win7. PR_InitializeNetAddr doesn't full memset 0 the ipv6 structure [1]. Therefore, PRNetAddr.ipv6.scope_id will have random value that cause PR_Bind fail. Test passed after manually memset 0 the PRNetAddr.

[1] http://searchfox.org/mozilla-central/source/nsprpub/pr/src/misc/prnetdb.c#1408
@mcmanus, `PR_InitializeNetAddr(PR_IpAddrNull, aPort, &prAddr)` will only memset 0 the ipv4 struct, however, ipv6 struct is larger than that. The last field of PRNetAddr.ipv6, i.e. scope_id, is most likely to have random value. Do you think this is something we should fix in NSPR?
Flags: needinfo?(mcmanus)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #25)
> @mcmanus, `PR_InitializeNetAddr(PR_IpAddrNull, aPort, &prAddr)` will only
> memset 0 the ipv4 struct, however, ipv6 struct is larger than that. The last
> field of PRNetAddr.ipv6, i.e. scope_id, is most likely to have random value.
> Do you think this is something we should fix in NSPR?

good find.

yes it should be fixed in nspr.. can you also provide a wrapper for gecko that zeroes it before calling nspr. That way we don't have to worry about syncing the library. (I'm assuming gecko is allocating the structure so its got a chance to do that. not sure of the stack)
Flags: needinfo?(mcmanus)
Pushed by schien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0de6247a69de
Part 1, not allow non-loopback UDP socket be created in offline mode. r=drno
https://hg.mozilla.org/integration/autoland/rev/c858f870c6d7
Part 2, test cases. r=drno
(In reply to Patrick McManus [:mcmanus] from comment #28)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #25)
> > @mcmanus, `PR_InitializeNetAddr(PR_IpAddrNull, aPort, &prAddr)` will only
> > memset 0 the ipv4 struct, however, ipv6 struct is larger than that. The last
> > field of PRNetAddr.ipv6, i.e. scope_id, is most likely to have random value.
> > Do you think this is something we should fix in NSPR?
> 
> good find.
> 
> yes it should be fixed in nspr.. can you also provide a wrapper for gecko
> that zeroes it before calling nspr. That way we don't have to worry about
> syncing the library. (I'm assuming gecko is allocating the structure so its
> got a chance to do that. not sure of the stack)

Bug 1330490 is filed for the NSPR fix, patch uploaded for review as well. Not sure about the exact code should look like for the wrapper, however I've file bug 1330543 to follow up.
https://hg.mozilla.org/mozilla-central/rev/0de6247a69de
https://hg.mozilla.org/mozilla-central/rev/c858f870c6d7
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.