1326483 - UDP socket child allows network connectivity in offline mode

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Nils Ohlmeier [:drno]]

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.

Comment 1

[Nils Ohlmeier [:drno]]

@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.

Comment 2

[Patrick McManus [:mcmanus]]

makes sense to me nils. thanks for noting the localhost bit

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I'll recommend to add offline mode check in nsUDPSocket since there are usages that directly access nsUDPSocket without going through IPC.

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.

Review commit: https://reviewboard.mozilla.org/r/102020/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102020/

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8823495 [details]
Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102020/diff/1-2/

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

File bug 1328504 for DevTool and bug 1328506 for MulticastDNS.jsm according to comment #6.

Comment 8

[Nils Ohlmeier [: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/#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 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 10

[Patrick McManus [: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

Comment 11

[Nils Ohlmeier [: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|.

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1326483 - Part 2, test cases.

Review commit: https://reviewboard.mozilla.org/r/102440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102440/

Comment 13

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8823495 [details]
Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102020/diff/2-3/

Comment 14

[Patrick McManus [:mcmanus]]

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 :)

Comment 15

[Nils Ohlmeier [: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

Comment 16

[Nils Ohlmeier [:drno]]

Comment on attachment 8823962 [details]
Bug 1326483 - Part 2, test cases.

https://reviewboard.mozilla.org/r/102440/#review103268

Comment 17

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8823495 [details]
Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102020/diff/3-4/

Comment 18

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8823962 [details]
Bug 1326483 - Part 2, test cases.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102440/diff/1-2/

Comment 19

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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?

Comment 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Maybe related to bug 892019?

Comment 21

[Nils Ohlmeier [:drno]]

(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 :-(

Comment 22

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8823495 [details]
Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102020/diff/4-5/

Comment 23

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8823962 [details]
Bug 1326483 - Part 2, test cases.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102440/diff/2-3/

Comment 24

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(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

Comment 25

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@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?

Comment 26

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8823495 [details]
Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102020/diff/5-6/

Comment 27

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8823962 [details]
Bug 1326483 - Part 2, test cases.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102440/diff/3-4/

Comment 28

[Patrick McManus [: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)

Comment 29

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8823495 [details]
Bug 1326483 - Part 1, not allow non-loopback UDP socket be created in offline mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102020/diff/6-7/

Comment 30

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8823962 [details]
Bug 1326483 - Part 2, test cases.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102440/diff/4-5/

Comment 31

[Pulsebot]

Pushed by [e-mail]:
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

Comment 32

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(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.

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0de6247a69de
https://hg.mozilla.org/mozilla-central/rev/c858f870c6d7