Closed
Bug 936320
Opened 11 years ago
Closed 10 years ago
_PR_INET6_PROBE is problematic with sandboxed e10s
Categories
(NSPR :: NSPR, defect, P2)
Tracking
(e10slater)
RESOLVED
FIXED
4.10.4
Tracking | Status | |
---|---|---|
e10s | later | --- |
People
(Reporter: jld, Assigned: wtc)
References
Details
Attachments
(4 files, 2 obsolete files)
(deleted),
patch
|
jld
:
review+
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
kang
:
review+
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
kang
:
review+
cyu
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jld
:
review+
Sylvestre
:
approval-mozilla-aurora+
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•11 years ago
|
||
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
Reporter | ||
Comment 2•11 years ago
|
||
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?
Reporter | ||
Comment 3•11 years ago
|
||
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.
Reporter | ||
Comment 4•11 years ago
|
||
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]
Reporter | ||
Comment 5•11 years ago
|
||
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.
Assignee | ||
Comment 6•11 years ago
|
||
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.
Reporter | ||
Comment 7•11 years ago
|
||
(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.
Assignee | ||
Comment 8•11 years ago
|
||
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)
Assignee | ||
Comment 9•11 years ago
|
||
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)
Reporter | ||
Comment 10•11 years ago
|
||
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)
Reporter | ||
Comment 11•11 years ago
|
||
(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.
Reporter | ||
Comment 12•11 years ago
|
||
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+
Assignee | ||
Comment 13•11 years ago
|
||
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)
Reporter | ||
Comment 14•11 years ago
|
||
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)
Reporter | ||
Comment 15•11 years ago
|
||
Here's an alternative approach — "preloading" the IPv6 probe by calling PR_StringToNetAddr from an appropriate place in ContentChild.
Attachment #8364021 -
Flags: review?(gdestuynder)
Reporter | ||
Comment 16•11 years ago
|
||
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+
Assignee | ||
Comment 19•11 years ago
|
||
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 20•11 years ago
|
||
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+
Assignee | ||
Comment 21•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [leave open]
Target Milestone: --- → 4.10.3
Comment 22•11 years ago
|
||
Reporter | ||
Comment 23•11 years ago
|
||
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.
Reporter | ||
Comment 24•11 years ago
|
||
(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.
Assignee | ||
Comment 25•11 years ago
|
||
Jed: let's give this a try. Thanks.
Attachment #8362143 -
Attachment is obsolete: true
Attachment #8379199 -
Flags: review?(jld)
Assignee | ||
Comment 26•11 years ago
|
||
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)
Reporter | ||
Comment 27•11 years ago
|
||
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-
Comment 28•11 years ago
|
||
(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?
Reporter | ||
Comment 29•11 years ago
|
||
(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.
Assignee | ||
Comment 30•11 years ago
|
||
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)
Reporter | ||
Updated•11 years ago
|
Attachment #8379999 -
Flags: review?(jld)
Attachment #8379999 -
Flags: review+
Attachment #8379999 -
Flags: approval-mozilla-aurora?
Reporter | ||
Comment 31•11 years ago
|
||
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+
Assignee | ||
Comment 32•11 years ago
|
||
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)
Assignee | ||
Comment 33•11 years ago
|
||
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+
Assignee | ||
Comment 34•11 years ago
|
||
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+
Assignee | ||
Updated•11 years ago
|
Target Milestone: 4.10.3 → 4.10.4
Assignee | ||
Comment 35•11 years ago
|
||
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)
Reporter | ||
Comment 37•11 years ago
|
||
(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)
Assignee | ||
Comment 38•11 years ago
|
||
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
Assignee | ||
Comment 39•11 years ago
|
||
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
Updated•11 years ago
|
Attachment #8379999 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
tracking-e10s:
--- → +
Updated•11 years ago
|
Assignee | ||
Comment 41•10 years ago
|
||
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: 10 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Reporter | ||
Comment 42•10 years ago
|
||
(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)
Comment 43•10 years ago
|
||
Thanks, that should be sufficient.
You need to log in
before you can comment on or make changes to this bug.
Description
•