Closed Bug 936320 Opened 6 years ago Closed 5 years ago

_PR_INET6_PROBE is problematic with sandboxed e10s

Categories

(NSPR :: NSPR, defect, P2)

4.10.2
ARM
Gonk (Firefox OS)
defect

Tracking

(e10slater)

RESOLVED FIXED
4.10.4
Tracking Status
e10s later ---

People

(Reporter: jld, Assigned: wtc)

References

Details

Attachments

(4 files, 2 obsolete files)

Recently I was trying to run a B2G debug build with seccomp-bpf sandboxing, and it caught a content process trying to call socket(), apparently to probe for IPv6 support (via _pr_test_ipv6_socket) to determine whether PR_StringToNetAddr should try to parse a string as a numeric v6 address.

We don't want a potentially compromised content process to be creating Internet-domain sockets on its own, but I assume there are reasons why NSPR is doing what it's doing and we shouldn't just disable this functionality.
We won't be able to enable sandboxing for the B2G emulator if it will break debug builds on TBPL, so this blocks that.
Blocks: 932098
Context:

#0  socket () at bionic/libc/arch-arm/syscalls/socket.S:10
#1  0x42c910bc in _pr_test_ipv6_socket () at /home/jld/src/B2G/gecko/nsprpub/pr/src/pthreads/ptio.c:3423
#2  0x42c83a02 in _pr_probe_ipv6_presence () at /home/jld/src/B2G/gecko/nsprpub/pr/src/io/pripv6.c:273
#3  0x42c83a0c in _pr_init_ipv6 () at /home/jld/src/B2G/gecko/nsprpub/pr/src/io/pripv6.c:284
#4  0x42c83226 in PR_CallOnce (once=0x42d886c0, func=0x42c83a05 <_pr_init_ipv6>)
    at /home/jld/src/B2G/gecko/nsprpub/pr/src/misc/prinit.c:775
#5  0x42c8395e in _pr_ipv6_is_present () at /home/jld/src/B2G/gecko/nsprpub/pr/src/io/pripv6.c:324
#6  0x42c8836e in PR_StringToNetAddr (string=0xbe9bfba4 "homescreen.gaiamobile.org", addr=0xbe9bfae0)
    at /home/jld/src/B2G/gecko/nsprpub/pr/src/misc/prnetdb.c:2232
#7  0x40773162 in nsEffectiveTLDService::GetBaseDomainInternal (this=<value optimized out>, aHostname=..., aAdditionalParts=1, 
    aBaseDomain=<value optimized out>) at /home/jld/src/B2G/gecko/netwerk/dns/nsEffectiveTLDService.cpp:247
#8  0x40773490 in nsEffectiveTLDService::GetBaseDomain (this=0x43ef8f40, aURI=<value optimized out>, aAdditionalParts=0, 
    aBaseDomain=...) at /home/jld/src/B2G/gecko/netwerk/dns/nsEffectiveTLDService.cpp:172
#9  0x40fc2db2 in GetNonDetachedWindowDomainsEnumerator (aId=<value optimized out>, aWindow=<value optimized out>, 
    aClosure=0xbe9bfd18) at /home/jld/src/B2G/gecko/dom/base/nsWindowMemoryReporter.cpp:741
[hash table implementation elided]
#13 nsWindowMemoryReporter::CheckForGhostWindows (this=0x44017340, aOutGhostIDs=0x0)
    at /home/jld/src/B2G/gecko/dom/base/nsWindowMemoryReporter.cpp:789
#14 0x40fc32b4 in nsWindowMemoryReporter::CheckForGhostWindowsCallback (this=0x403ad080)
    at /home/jld/src/B2G/gecko/dom/base/nsWindowMemoryReporter.cpp:643


Questions raised by this:

1. In the unlikely(?) event of running on a system with IPv6 disabled in its kernel (such that simply creating a v6 socket fails), do we really need to refuse to parse an explicit numeric IPv6 address?

2. Should nsEffectiveTLDService really be using a full address parser to check whether the string it has is a DNS name rather than an address?

3. Does the window-objects memory reporter need to be using nsEffectiveTLDService like this in a content process?
It gets worse.

(gdb) bt
#0  socket () at bionic/libc/arch-arm/syscalls/socket.S:10
#1  0x42c910bc in _pr_test_ipv6_socket () at /home/jld/src/B2G/gecko/nsprpub/pr/src/pthreads/ptio.c:3423
#2  0x42c83a02 in _pr_probe_ipv6_presence () at /home/jld/src/B2G/gecko/nsprpub/pr/src/io/pripv6.c:273
#3  0x42c83a0c in _pr_init_ipv6 () at /home/jld/src/B2G/gecko/nsprpub/pr/src/io/pripv6.c:284
#4  0x42c83226 in PR_CallOnce (once=0x42d886c0, func=0x42c83a05 <_pr_init_ipv6>)
    at /home/jld/src/B2G/gecko/nsprpub/pr/src/misc/prinit.c:775
#5  0x42c8395e in _pr_ipv6_is_present () at /home/jld/src/B2G/gecko/nsprpub/pr/src/io/pripv6.c:324
#6  0x42c8836e in PR_StringToNetAddr (string=0xbeb13780 "homescreen.gaiamobile.org", addr=0xbeb136b8)
    at /home/jld/src/B2G/gecko/nsprpub/pr/src/misc/prnetdb.c:2232
#7  0x40773162 in nsEffectiveTLDService::GetBaseDomainInternal (this=<value optimized out>, aHostname=..., aAdditionalParts=-1, 
    aBaseDomain=<value optimized out>) at /home/jld/src/B2G/gecko/netwerk/dns/nsEffectiveTLDService.cpp:247
#8  0x407732b0 in nsEffectiveTLDService::GetNextSubDomain (this=0x43ff8f40, aHostname=<value optimized out>, aBaseDomain=...)
    at /home/jld/src/B2G/gecko/netwerk/dns/nsEffectiveTLDService.cpp:219
#9  0x409d4618 in GetNextSubDomainForHost (this=0x4416d190, aHost=..., aAppId=<value optimized out>, 
    aIsInBrowserElement=<value optimized out>, aType=36, aExactHostMatch=<value optimized out>)
    at /home/jld/src/B2G/gecko/extensions/cookie/nsPermissionManager.cpp:169
#10 nsPermissionManager::GetPermissionHashKey (this=0x4416d190, aHost=..., aAppId=<value optimized out>, 
    aIsInBrowserElement=<value optimized out>, aType=36, aExactHostMatch=<value optimized out>)
    at /home/jld/src/B2G/gecko/extensions/cookie/nsPermissionManager.cpp:1227
#11 0x409d4adc in nsPermissionManager::CommonTestPermission (this=0x4416d190, aPrincipal=0x44359d00, aType=<value optimized out>, 
    aPermission=0xbeb13a10, aExactHostMatch=false, aIncludingSession=true)
    at /home/jld/src/B2G/gecko/extensions/cookie/nsPermissionManager.cpp:1155
#12 0x409d4b8a in nsPermissionManager::TestPermissionFromPrincipal (this=<value optimized out>, aPrincipal=<value optimized out>, 
    aType=0x0, aPermission=0x0) at /home/jld/src/B2G/gecko/extensions/cookie/nsPermissionManager.cpp:1026
#13 0x409d1926 in nsPermissionManager::TestPermissionFromWindow (this=0x4416d190, aWindow=<value optimized out>, 
    aType=0x4222afc4 "idle", aPermission=0xbeb13a10) at /home/jld/src/B2G/gecko/extensions/cookie/nsPermissionManager.cpp:1018
#14 0x40f98abc in mozilla::dom::Navigator::CheckPermission (aWindow=0x4418c7f0, aType=0x4222afc4 "idle")
    at /home/jld/src/B2G/gecko/dom/base/Navigator.cpp:1466
#15 0x40f98fd8 in mozilla::dom::Navigator::HasIdleSupport (aGlobal=<value optimized out>)
    at /home/jld/src/B2G/gecko/dom/base/Navigator.cpp:1663


What I don't understand is why this doesn't occur on a non-debug build.  If PR_StringToNetAddr is called before sandbox startup (on any input) then that would prevent this, but I don't see why debug build wouldn't do the same.  Maybe the mystery lookup is happening concurrently with the actions leading up to enabling the sandbox, so it's timing-sensitive?  (Main thread vs. I/O thread?)

This also suggests a hacky workaround: doing PR_StringToNetAddr("::", &ignored) or similar before starting the sandbox.
Well, I've found the mysterious non-debug early call to PR_StringToNetAddr:

(gdb) bt
#0  PR_StringToNetAddr (string=0xbe935e0c "localhost", addr=0xbe935d88) at /home/jld/src/B2G/gecko/nsprpub/pr/src/misc/prnetdb.c:2220
#1  0x407003ac in nsProtocolProxyService::LoadHostFilters (this=0x438976a0, filters=<value optimized out>)
    at /home/jld/src/B2G/gecko/netwerk/base/src/nsProtocolProxyService.cpp:1345
#2  0x40701b1a in nsProtocolProxyService::PrefsChanged (this=0x438976a0, prefBranch=0x433de5c8, pref=0x0)
    at /home/jld/src/B2G/gecko/netwerk/base/src/nsProtocolProxyService.cpp:530
#3  0x40701d28 in nsProtocolProxyService::Init (this=0x438976a0)
    at /home/jld/src/B2G/gecko/netwerk/base/src/nsProtocolProxyService.cpp:402
#4  0x406e5f26 in nsProtocolProxyServiceConstructor (aOuter=<value optimized out>, aIID=..., aResult=0xbe935f34)
    at /home/jld/src/B2G/gecko/netwerk/build/nsNetModule.cpp:65
#5  0x40694824 in mozilla::GenericFactory::CreateInstance (this=<value optimized out>, aOuter=<value optimized out>, 
    aIID=<value optimized out>, aResult=0x0) at /home/jld/src/B2G/gecko/xpcom/glue/GenericFactory.cpp:16
#6  0x406c0dc0 in nsComponentManagerImpl::CreateInstance (this=<value optimized out>, aClass=<value optimized out>, aDelegate=0x0, 
    aIID=..., aResult=0xbe935f34) at /home/jld/src/B2G/gecko/xpcom/components/nsComponentManager.cpp:995
#7  0x406c2204 in nsComponentManagerImpl::GetService (this=0x40339c00, aClass=..., aIID=..., result=<value optimized out>)
    at /home/jld/src/B2G/gecko/xpcom/components/nsComponentManager.cpp:1244
#8  0x40c7b73a in nsJSCID::GetService (this=<value optimized out>, iidval=<value optimized out>, cx=0x403371c0, 
    optionalArgc=<value optimized out>, retval=0xbe936088) at /home/jld/src/B2G/gecko/js/xpconnect/src/XPCJSID.cpp:798
#9  0x406c8e3e in NS_InvokeByIndex (that=<value optimized out>, methodIndex=<value optimized out>, paramCount=<value optimized out>, 
    params=<value optimized out>) at /home/jld/src/B2G/gecko/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:164
#10 0x40c8c84c in CallMethodHelper::Invoke (ccx=<value optimized out>, mode=<value optimized out>)
    at /home/jld/src/B2G/gecko/js/xpconnect/src/XPCWrappedNative.cpp:2561
#11 CallMethodHelper::Call (ccx=<value optimized out>, mode=<value optimized out>)
    at /home/jld/src/B2G/gecko/js/xpconnect/src/XPCWrappedNative.cpp:1901
#12 XPCWrappedNative::CallMethod (ccx=<value optimized out>, mode=<value optimized out>)
    at /home/jld/src/B2G/gecko/js/xpconnect/src/XPCWrappedNative.cpp:1867
#13 0x40c90952 in XPC_WN_CallMethod (cx=0x403371c0, argc=1, vp=<value optimized out>)
    at /home/jld/src/B2G/gecko/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1300
#14 0x41630f6e in CallJSNative (cx=0x403371c0, args=..., construct=<value optimized out>)
    at /home/jld/src/B2G/gecko/js/src/../../js/src/jscntxtinlines.h:220
#15 Invoke (cx=0x403371c0, args=..., construct=<value optimized out>) at /home/jld/src/B2G/gecko/js/src/vm/Interpreter.cpp:465
#16 0x41635fd2 in Interpret (cx=0x403371c0, state=<value optimized out>) at /home/jld/src/B2G/gecko/js/src/vm/Interpreter.cpp:2614
#17 0x41637c46 in RunScript (cx=0x403371c0, state=...) at /home/jld/src/B2G/gecko/js/src/vm/Interpreter.cpp:422
#18 0x41637f2c in ExecuteKernel (cx=0x403371c0, script=..., scopeChainArg=<value optimized out>, rval=0x0)
    at /home/jld/src/B2G/gecko/js/src/vm/Interpreter.cpp:619
#19 js::Execute (cx=0x403371c0, script=..., scopeChainArg=<value optimized out>, rval=0x0)
    at /home/jld/src/B2G/gecko/js/src/vm/Interpreter.cpp:656
#20 0x41584552 in JS_ExecuteScript (cx=0x403371c0, objArg=0x4391f160, scriptArg=<value optimized out>, rval=0x0)
    at /home/jld/src/B2G/gecko/js/src/jsapi.cpp:4761
#21 0x40e34ba8 in nsFrameScriptExecutor::LoadFrameScriptInternal (this=<value optimized out>, aURL=<value optimized out>, 
    aRunInGlobalScope=<value optimized out>) at /home/jld/src/B2G/gecko/content/base/src/nsFrameMessageManager.cpp:1423
#22 0x40c35238 in mozilla::dom::TabChild::RecvLoadRemoteScript (this=0x403af060, aURL=..., aRunInGlobalScope=@0xbe93696f)
    at /home/jld/src/B2G/gecko/dom/ipc/TabChild.cpp:2109
#23 0x40c365ee in mozilla::dom::TabChild::PreloadSlowThings () at /home/jld/src/B2G/gecko/dom/ipc/TabChild.cpp:231
#24 0x40c2bc90 in PreloadSlowThings (this=0x40344c18, version=<value optimized out>, buildID=..., name=..., UAName=...)
    at /home/jld/src/B2G/gecko/dom/ipc/ContentChild.cpp:1343
#25 mozilla::dom::ContentChild::RecvAppInfo (this=0x40344c18, version=<value optimized out>, buildID=..., name=..., UAName=...)
    at /home/jld/src/B2G/gecko/dom/ipc/ContentChild.cpp:1370

JS stack:

0 anonymous(global = [object ContentFrameMessageManager]) ["chrome://global/content/preload.js":59]
1 <TOP LEVEL> ["chrome://global/content/preload.js":106]
    this = [object ContentFrameMessageManager]
In ContentChild::RecvAppInfo, before the PreloadSlowThings() call:

    if (!Preferences::GetBool("dom.ipc.processPrelaunch.enabled", false)) {
        return true;
    }

And in b2g/app/b2g.js:

#ifndef DEBUG
// Enable pre-launching content processes for improved startup time
// (hiding latency).
pref("dom.ipc.processPrelaunch.enabled", true);
// Wait this long before pre-launching a new subprocess.
pref("dom.ipc.processPrelaunch.delayMs", 5000);
#endif

And that's why this is a debug-only crash.


All that said, the first two questions I raised in comment #2 remain valid.
We may be able to undefine _PR_INET6_PROBE for Linux. Right now
it is defined in nsprpub/pr/include/md/_linux.h. That code was
added in year 2000:
https://bugzilla.mozilla.org/show_bug.cgi?id=25153

It is possible that that socket(AF_INET6, SOCK_STREAM, 0) test
always succeeds on Linux and its derivatives today, so it is no
longer necessary.
(In reply to Wan-Teh Chang from comment #6)
> We may be able to undefine _PR_INET6_PROBE for Linux. Right now
> it is defined in nsprpub/pr/include/md/_linux.h. That code was
> added in year 2000:
> https://bugzilla.mozilla.org/show_bug.cgi?id=25153
> 
> It is possible that that socket(AF_INET6, SOCK_STREAM, 0) test
> always succeeds on Linux and its derivatives today, so it is no
> longer necessary.

Maybe not — building the Linux kernel with IPv6 disabled or as a loadable module seems to still be possible in theory (I found a patch from September 2013 to fix a build breakage affecting that case, so someone's doing it).  But I don't know that any mainstream distribution (as opposed to, e.g., an embedded system on a v4-only private network) would actually disable it these days.
Jed: if PR_StringToNetAddr is the only NSPR function that you need to call
sandboxed, we can try this patch.

Apply the patch to nsprpub/pr/src/misc/prnetdb.c in the Mozilla source tree.
This patch makes PR_StringToNetAddr bypass the _pr_ipv6_is_present() test as
long as the IP address literal doesn't contain '%', which is only used for
IPv6 scope IDs.

Re: undefining _PR_INET6_PROBE: is there a compiler predefined macro for
detecting Firefox OS?
Attachment #8361947 - Flags: review?(jld)
This patch undefines _PR_INET6_PROBE for Linux.

Another idea is to find an alternative test for IPv6 support
that works in the sandbox. Is there some special file (such
as a file under /proc) that we can read to detect IPv6 support?
Attachment #8362143 - Flags: review?(jld)
Comment on attachment 8361947 [details] [diff] [review]
Change PR_StringToNetAddr to try pr_StringToNetAddrFB first

Review of attachment 8361947 [details] [diff] [review]:
-----------------------------------------------------------------

This seems to work (although I haven't tested more than booting the phone and viewing a simple webpage).  But I don't like that this code is still present and could potentially be called.

It seems to me that we should either disable the probe entirely or arrange to run it earlier, before the sandbox is enabled.
Attachment #8361947 - Flags: review?(jld)
(In reply to Wan-Teh Chang from comment #8)
> Re: undefining _PR_INET6_PROBE: is there a compiler predefined macro for
> detecting Firefox OS?

MOZ_WIDGET_GONK.  But there's also ongoing work to use the same sandboxing mechanism on desktop Firefox on Linux, and that might run into the same problem.
Comment on attachment 8362143 [details] [diff] [review]
[Doesn't work] Undefine _PR_INET6_PROBE for Linux

Review of attachment 8362143 [details] [diff] [review]:
-----------------------------------------------------------------

This seems to work… but I don't know how it would affect all possible cases of projects using NSPR on Linux.

As for opening files in /proc, the longer-term plan is to prevent content processes from accessing the filesystem directly, which means no open().

Which, if we don't want to commit this patch as-is, brings us back to changing something somewhere to test for IPv6 support earlier in content process startup.
Attachment #8362143 - Flags: review?(jld) → feedback+
Jed: if we can call access("/proc/net/if_inet6", F_OK) in the sandbox,
this patch should also solve the problem. Could you test it? Thanks.

This test is documented at
http://www.tldp.org/HOWTO/Linux+IPv6-HOWTO/systemcheck-kernel.html#AEN719
Attachment #8364014 - Flags: review?(jld)
Comment on attachment 8364014 [details] [diff] [review]
An alternative test for IPv6 support on Linux

Handing off the review — access() is currently in the list of syscalls we're trying to remove from the whitelist at some point, but this would be an improvement over socket().
Attachment #8364014 - Flags: review?(jld) → review?(gdestuynder)
Here's an alternative approach — "preloading" the IPv6 probe by calling PR_StringToNetAddr from an appropriate place in ContentChild.
Attachment #8364021 - Flags: review?(gdestuynder)
I should add that that's a rough draft and I haven't tried building on non-b2g or with --disable-content-sandbox yet.
Comment on attachment 8364021 [details] [diff] [review]
bug936320-preload-nspr-v6-probe-hg0.diff

Review of attachment 8364021 [details] [diff] [review]:
-----------------------------------------------------------------

from my point of view it's fine, but, as we're also preloading elsewhere (such as with NUWA) there might be a more consistent way to do this.

Cervantes, do you have some feedback regarding this?
Attachment #8364021 - Flags: review?(gdestuynder)
Attachment #8364021 - Flags: review+
Attachment #8364021 - Flags: feedback?(cyu)
Comment on attachment 8364014 [details] [diff] [review]
An alternative test for IPv6 support on Linux

Review of attachment 8364014 [details] [diff] [review]:
-----------------------------------------------------------------

As :jld wrote - this does solve the socket() issue and reduces the whitelist, from that point of view, it's fine.
However, it's delaying the issue for when we remove access().

If we're taking too long to fix this/we're blocked, I would argue that this (attachment 8364014 [details] [diff] [review]) patch should be taken instead, as access() is a much lesser evil.
Attachment #8364014 - Flags: review?(gdestuynder) → review+
Comment on attachment 8364014 [details] [diff] [review]
An alternative test for IPv6 support on Linux

Patch checked in: https://hg.mozilla.org/projects/nspr/rev/44cf8247f24b
Attachment #8364014 - Flags: checked-in+
Comment on attachment 8364021 [details] [diff] [review]
bug936320-preload-nspr-v6-probe-hg0.diff

We have PreloadSlowThings() called in ContentChild::RecvAppInfo(). This is somewhat strange and unclear. There isn't a clear and explicit stage to perform all the preload job in the content process. Doing unsandboxable things right before sandboxing should work. I think it's OK to do the preload before sandboxing. If there will more stuff to preload, especially after the enablement of the Nuwa template process, then we'd better centralize the preload work to make it clearer.
Attachment #8364021 - Flags: feedback?(cyu) → feedback+
Comment on attachment 8364014 [details] [diff] [review]
An alternative test for IPv6 support on Linux

Patch pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75c140ce2a5d
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [leave open]
Target Milestone: --- → 4.10.3
No longer blocks: 932098
Depends on: 971152
It turns out that the socket(PF_INET6, ...) test has another problem on Android and B2G: the call will fail with EACCES unless the calling process has uid 0 or is in group 3003 (AID_INET).  This doesn't include Fennec (or, in general, Android apps with permission to access the Internet directly), but it does affect non-preallocated non-Nuwa-spawned B2G content processes — even without seccomp sandboxing.
(In reply to Jed Davis [:jld] from comment #23)
> It turns out that the socket(PF_INET6, ...) test has another problem on
> Android and B2G: the call will fail with EACCES unless the calling process
> has uid 0 or is in group 3003 (AID_INET).  This doesn't include Fennec (or,
> in general, Android apps with permission to access the Internet directly),
> but it does affect non-preallocated non-Nuwa-spawned B2G content processes —
> even without seccomp sandboxing.

…which actually doesn't matter, because (I assume?) the process whose belief about IPv6 is important for our name resolution is the parent process, which will necessarily have whatever permissions are needed for the socket test because it's the process doing the actual network communications.
Jed: let's give this a try. Thanks.
Attachment #8362143 - Attachment is obsolete: true
Attachment #8379199 - Flags: review?(jld)
Comment on attachment 8361947 [details] [diff] [review]
Change PR_StringToNetAddr to try pr_StringToNetAddrFB first

Review of attachment 8361947 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should also make this change. The current order
of these two code fragments was chosen arbitrarily. The
new order allows PR_StringToNetAdd to avoid system calls in
more cases.
Attachment #8361947 - Flags: review?(jld)
Comment on attachment 8379199 [details] [diff] [review]
Undefine _PR_INET6_PROBE and revert the alternative test for IPv6 support on Linux

I don't think we should undefine _PR_INET6_PROBE, if that would cause hosts with IPv6 disabled to send and wait for DNS AAAA queries.  At the very least I'd like that to be a separate change, so it can be backed out easily if it causes problems.

I do think we should back out the previous change, and uplift the backout to mozilla-aurora, to fix bug 971152.  Now that bug 974230 has landed, the socket() call won't crash with seccomp enabled, and Gecko 29 isn't used for B2G (desktop seccomp whitelists socket(), and isn't enabled by default in any case) so the uplift can't break anything.
Attachment #8379199 - Flags: review?(jld) → feedback-
(In reply to Jed Davis [:jld] from comment #27)
> Now that bug 974230 has landed, the
> socket() call won't crash with seccomp enabled,

Does this mean IPv6 will be always disabled in the content process?
(In reply to Masatoshi Kimura [:emk] from comment #28)
> (In reply to Jed Davis [:jld] from comment #27)
> > Now that bug 974230 has landed, the
> > socket() call won't crash with seccomp enabled,
> 
> Does this mean IPv6 will be always disabled in the content process?

Yes, but that was already the case before this bug — see comment #23 and comment #24.  But I don't know if that affects anything, given that the parent is responsible for name resolution and networking.
Jed:

OK, I split the patch into two patches. This patch reverts the
alternative test for IPv6 support on Linux.
Attachment #8379199 - Attachment is obsolete: true
Attachment #8379999 - Flags: review?(jld)
Attachment #8379999 - Flags: review?(jld)
Attachment #8379999 - Flags: review+
Attachment #8379999 - Flags: approval-mozilla-aurora?
Comment on attachment 8361947 [details] [diff] [review]
Change PR_StringToNetAddr to try pr_StringToNetAddrFB first

Review of attachment 8361947 [details] [diff] [review]:
-----------------------------------------------------------------

And if we still want to commit this patch at this point, I don't see why not.
Attachment #8361947 - Flags: review?(jld) → review+
Comment on attachment 8362143 [details] [diff] [review]
[Doesn't work] Undefine _PR_INET6_PROBE for Linux

Jed:

Your concern about NSPR requesting DNS AAAA records unnecessarily
is addressed by the PR_AI_ADDRCONFIG flag for PR_GetAddrInfoByName
rather than by the _PR_INET6_PROBE macro:
http://mxr.mozilla.org/mozilla-central/ident?i=PR_AI_ADDRCONFIG

_PR_INET6_PROBE is probably not useful today. _PR_INET6_PROBE was
added in the old world when it was recommended that we just open
AF_INET6 sockets for both IPv4 (via IPv4-mapped IPv6 addresses)
and IPv6. In the new world, we call getaddrinfo() and open the
socket of the right address family. So I suspect that undefining
_PR_INET6_PROBE for Linux will be an uneventful change.

If you are uncomfortable reviewing this patch, please feel free
to decline the review request.
Attachment #8362143 - Attachment is obsolete: false
Attachment #8362143 - Flags: review?(jld)
Comment on attachment 8379999 [details] [diff] [review]
Revert the alternative test for IPv6 support on Linux

Patch checked in: https://hg.mozilla.org/projects/nspr/rev/ad00f36fa460
Attachment #8379999 - Flags: checked-in+
Comment on attachment 8361947 [details] [diff] [review]
Change PR_StringToNetAddr to try pr_StringToNetAddrFB first

Patch checked in: https://hg.mozilla.org/projects/nspr/rev/b68b651fb7c3
Attachment #8361947 - Flags: checked-in+
Target Milestone: 4.10.3 → 4.10.4
Comment on attachment 8362143 [details] [diff] [review]
[Doesn't work] Undefine _PR_INET6_PROBE for Linux

I found that Linux still needs _PR_INET6_PROBE. It is easy to disable
IPv6 support in Linux. For example, the Debian IPv6 project page
provides a simple two-step procedure:

    How to turn off IPv6

    1. Append ipv6.disable=1 to the GRUB_CMDLINE_LINUX variable in
       /etc/default/grub.
    2. Run update-grub and reboot.

https://wiki.debian.org/DebianIPv6#How_to_turn_off_IPv6

After I did that on Ubuntu 13.10, the /proc/net/if_inet6 is gone, and
socket(AF_INET6, SOCK_STREAM, 0) fails with errno 97 (EAFNOSUPPORT).
PR_OpenTCPSocket(PR_AF_INET6) still needs to work in a reduced,
"IPv4-mapped" mode in this case. Until we drop that feature, we still
need to do IPv6 presence test on Linux.
Attachment #8362143 - Attachment description: Undefine _PR_INET6_PROBE for Linux → [Doesn't work] Undefine _PR_INET6_PROBE for Linux
Attachment #8362143 - Attachment is obsolete: true
Attachment #8362143 - Flags: review?(jld)
can you fill the aurora uplift "form"? thanks
Flags: needinfo?(jld)
(In reply to Sylvestre Ledru [:sylvestre] from comment #36)
> can you fill the aurora uplift "form"? thanks

Sorry about that — the form doesn't appear when setting the flag from the review interface.

Also, it seems that this hasn't landed on m-c yet, only in the "upstream" NSPR repository, so that flag may have been premature.  I'll fill it out anyway:

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 971152 (except I think this might be backwards — bug 971152 was caused by the previous commit to mitigate this bug, which attachment 8379999 [details] [diff] [review] backs out).
User impact if declined: No IPv6 support for Linux desktop Firefox users using certain security features (again, see bug 971152).
Testing completed (on m-c, etc.): See above; this needs to actually get to m-c first (at which point Buildbot/TBPL will take care of it, as bug 932098 is resolved).
Risk to taking this patch (and alternatives if risky): This is reverting back to code that's been in NSPR since the year 2000, so it should be quite safe.  The original bug is specific to B2G, which isn't supported on Gecko 29.
String or IDL/UUID changes made by this patch: None.
Flags: needinfo?(jld)
Comment on attachment 8379999 [details] [diff] [review]
Revert the alternative test for IPv6 support on Linux

Review of attachment 8379999 [details] [diff] [review]:
-----------------------------------------------------------------

Patch pushed to mozilla-inbound as part of the NSPR_4_10_4_BETA4 update:
https://hg.mozilla.org/integration/mozilla-inbound/rev/65f75ee1c34f
Comment on attachment 8379999 [details] [diff] [review]
Revert the alternative test for IPv6 support on Linux

Review of attachment 8379999 [details] [diff] [review]:
-----------------------------------------------------------------

Patch pushed to mozilla-central as part of NSPR_4_10_4_BETA:
https://hg.mozilla.org/mozilla-central/rev/65f75ee1c34f
Attachment #8379999 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Did the uplift ever happen?
Flags: needinfo?(jld)
Florian:

I'm sorry that I left this bug in a confusing state.

As an NSPR bug, we did the following:
- I reverted the alternative test for IPv6 support on Linux
  because it introduced bug 971152.
- We can't undefine  _PR_INET6_PROBE on Linux because it is
  easy to disable IPv6 support in Linux (see comment 35).
- I changed PR_StringToNetAddr so that it doesn't need to
  perform the test for IPv6 support on Kinux most of the
  time.

I am not sure whether we should resolve this bug FIXED or
WONTFIX. But it should be closed.

To determine whether the uplift has happened, check the
nsprpub/TAG-INFO file in the source tree. It should be
NSPR_4_10_4_RTM or later. Which Mozilla hg repository do
you want me to check?
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
(In reply to Florian Bender from comment #40)
> Did the uplift ever happen?

No, it didn't.  Firefox 29 is affected by bug 971152; Firefox 30+ isn't.
Flags: needinfo?(jld)
Thanks, that should be sufficient.
You need to log in before you can comment on or make changes to this bug.