Status

()

Core
IPC
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Charles Collicutt, Assigned: Charles Collicutt)

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160309193552

Steps to reproduce:

Attempt to build Firefox on a Linux system without 'sys/sysctl.h' (e.g. using musl libc rather than glibc)


Actual results:

In file included from .../mozilla-central/ipc/chromium/src/third_party/libevent/./arc4random.c:62:0,
                  from .../mozilla-central/ipc/chromium/src/third_party/libevent/evutil_rand.c:104,
                  from .../mozilla-central/obj-firefox-x86_64-pc-linux-gnu/ipc/chromium/src/third_party/Unified_c_src_third_party0.c:101:
.../mozilla-central/obj-firefox-x86_64-pc-linux-gnu/dist/system_wrappers/sys/sysctl.h:3:29: fatal error: sys/sysctl.h: No such file or directory


Expected results:

No attempt should have been made to include sys/sysctl.h on a Linux system: the header file is not guaranteed to be present and even if it is present it shouldn't be used.

This bug is similar to Bug 1130175, which was resolved by including sysctl.h only on BSD/OS X systems.

It's also the same problem as Bug 1261414 - only it's simpler here as we already have separate configs for Linux and other Unix systems.

From the description of Bug 1261414:

The man page for the Linux system call sysctl(2) says:
> don't call it: use of this system call has long been
> discouraged, and it is so unloved that it is likely to
> disappear in a future kernel version.  Since Linux 2.6.24,
> uses of this system call result in warnings in the kernel log.
> Remove it from your programs now; use the /proc/sys interface instead.
(Assignee)

Comment 1

2 years ago
Created attachment 8739763 [details] [diff] [review]
Don't use sysctl on Linux

This patch disables use of sysctl on Linux. The only effect this has is in arc4random.c where it removes one of the functions for gathering entropy.

Note that the code that calls that function currently has this comment:
> Apparently Linux is deprecating sysctl, and spewing warning messages when you try to use it.

We still have the function that uses /proc/sys rather than sysctl, which is the recommended method on Linux anyway. And we still have the function that reads /dev/srandom, /dev/urandom and /dev/random too.

Note that even with the sysctl function, libevent would also use the /dev/*random and /proc/sys functions too. I don't think we're losing anything by not using the deprecated function that spews warnings.
Attachment #8739763 - Flags: review?(wmccloskey)
Assignee: nobody → charles
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Whiteboard: btpp-active
(In reply to Charles Collicutt from comment #1)
> This patch disables use of sysctl on Linux. The only effect this has is in
> arc4random.c where it removes one of the functions for gathering entropy.

Do we even use that code anywhere?  If not, could we just remove it?  See also bug 1259218.
Comment on attachment 8739763 [details] [diff] [review]
Don't use sysctl on Linux

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

I tried to figure out how we use the arc4 stuff. It seems like we don't, but it might be hard to remove. For some reason libevent provides a DNS library (why?). And chromium's IPC code includes libevent. We could maybe get rid of the DNS code from libevent, but then we would have trouble taking merges for libevent from upstream. Maybe that doesn't matter, but we haven't previously made many changes to libevent afaik. So this seems like the right way to go to me. I've only looked at it briefly, though.
Attachment #8739763 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 4

2 years ago
As far as I can tell, Mozilla doesn't actually use the arc4random code (or libevent's DNS stuff). However, as you say, removing it would be a relatively large change to libevent. I think just changing a config value and leaving the rest as it is upstream is simpler.

Please could someone push this to the Try server for me?
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c645190e3695
(Assignee)

Comment 6

2 years ago
Thank you.
Keywords: checkin-needed
landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/b1374e24aef14ab023d8bb4775e0989eab4d463c (pulsebot missed to commit here)
Keywords: checkin-needed

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b1374e24aef1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.