No arc4random_addrandom symbol defined anymore

RESOLVED FIXED in mozilla28

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gaston, Assigned: gaston)

Tracking

unspecified
mozilla28
Other
OpenBSD
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
OpenBSD developers are in the progress of cleaning up the arc4random_* api which is used by ipc/chromium/src/third_party/libevent. Starting with libc.so.71.0 (and this commit : http://marc.info/?l=openbsd-cvs&m=138238762705209&w=2), arc4random_addrandom, and arc4random_stir are not available anymore and thus build fails:

: In function `evutil_secure_rng_add_bytes':
/var/buildslave-mozilla/mozilla-central-amd64/build/ipc/chromium/src/third_party/libevent/evutil_rand.c:145: undefined reference to `arc4random_addrandom'
/usr/bin/ld: libxul.so.1.0: hidden symbol `arc4random_addrandom' isn't defined

This has been solved in our deve/libevent2 port with the following patch:
http://www.openbsd.org/cgi-bin/cvsweb/ports/devel/libevent2/patches/patch-evutil_rand_c?rev=1.1;content-type=text%2Fplain (plus adding arc4random_addrandom to the list of functions checked by configure in AC_CHECK_FUNCS).

This is in the process of being pushed back at upstream's libevent, but in the meantime i'd like to fix my nightly builds on OpenBSD without resorting to --with-system-libevent. So, how is libevent patching done in the mozilla tree (i see a .patch in the parent dir a nd one in the libevent dir itself), and what kind of patch would be acceptable ? I might aswell #define ARC4RANDOM_NOADDRANDOM on OpenBSD in evutil_rand.c.
(Assignee)

Updated

5 years ago
Summary: No arc4random_random symbol defined anymore → No arc4random_addrandom symbol defined anymore
(Assignee)

Comment 1

5 years ago
BMO doesnt let me put https://sourceforge.net/p/levent/bugs/320/ in See Also:, but that's the ref to the upstreaming bug.
(Assignee)

Comment 2

5 years ago
Ben, Chris, any hindsight ?
Flags: needinfo?(cjones.bugs)
Flags: needinfo?(bent.mozilla)
The code is semantically a fork.  We patch it when we need to, preferably trying to pull in the change from chromium when possible.

Josh touched libevent last so he's in the best position to help with specific code changes.
Flags: needinfo?(joshmoz)
Flags: needinfo?(cjones.bugs)
Flags: needinfo?(bent.mozilla)
(Assignee)

Comment 4

5 years ago
josh, how can we move forward on this ? as of now m-a and m-c are broken on OpenBSD and if more failures pile on top my buildbot wont show them...

Comment 5

5 years ago
I just updated the libevent we got from Chromium with a copy of libevent2, I didn't take the updated copy from Chromium. Last time I checked they still used an older version.

The way to patch our copy is:

1. Apply your patch to the tree.

2. Include a copy of your patch in the source tree like this, so that future maintainers can see what happened:

http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/third_party/libevent/mac-arc4random-buf.patch

This one also happens to deal with arc4random.

3. Update these instructions:

http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/third_party/libevent/README.mozilla
Flags: needinfo?(joshmoz)
(Assignee)

Comment 6

5 years ago
Over time, more and more OSes will probably update the copy of arc4random they have in libc/etc, and thus remove arc4random_addrandom(). Should i add a configure check for arc4random_addrandom in AC_CHECK_FUNCS, and try to propagate it to ipc/chromium build via a DEFINE in moz.build there ? Or just not bother with it, and assume that from now on only OpenBSD doesnt ship arc4random_addrandom() and add it as a local patch to bsd/event2/event-config.h + evutil_rand.c ?
(Assignee)

Comment 7

5 years ago
Created attachment 829728 [details] [diff] [review]
only use arc4random_addrandom if it exists

That patch fixes the issue for me, but i'm not sure it's the best way yet so only askingfor feedback. (and yes, i'll add it as a separate patch outside of libevent tree + amend README.mozilla)

pushed to try in  https://tbpl.mozilla.org/?tree=Try&rev=e554799b41cd
Assignee: nobody → landry
Attachment #829728 - Flags: feedback?(mh+mozilla)
Attachment #829728 - Flags: feedback?(joshmoz)
(Assignee)

Comment 8

5 years ago
Thinking a bit more about this, the patch as-is is stupid since it would regress android/mac/linux by not calling arc4random_addrandom there too. So two options:
- add _EVENT_HAVE_ARC4RANDOM_ADDRANDOM to {android,linux,mac}/event2/event-config.h
- stop the #define maze and just put the call within #ifndef __OpenBSD__ for now

thoughts ?

Comment 9

5 years ago
Comment on attachment 829728 [details] [diff] [review]
only use arc4random_addrandom if it exists

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

I'm fine with this approach. The final patch here should include this, plus the patch itself as an added file, and mention of the patch in the readme.mozilla file. Thanks for taking care of this.
Attachment #829728 - Flags: feedback?(joshmoz) → feedback+

Comment 10

5 years ago
Actually, perhaps it is better to ifdef the entire function and callsites so nobody accidentally calls a function that does nothing.
(Assignee)

Comment 11

5 years ago
actually, it would regress android & mac, didnt notice linux has _EVENT_HAVE_ARC4RANDOM #undef'ed.

I've been told that arc4random_addrandom was actually making things worse randomization-wise that if it wasnt called, but i guess that's something to bring with upstream libevent.

(In reply to Josh Aas (Mozilla Corporation) from comment #10)
> Actually, perhaps it is better to ifdef the entire function and callsites so
> nobody accidentally calls a function that does nothing.

http://mxr.mozilla.org/mozilla/mozilla-central/search?string=evutil_secure_rng_add_bytes <- according to this, nothing calls it anyway......
Comment on attachment 829728 [details] [diff] [review]
only use arc4random_addrandom if it exists

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

Considering all the comments in this bug, why not just #define arc4random_addrandom(...) in openbsd's event-config.h?
Attachment #829728 - Flags: feedback?(mh+mozilla)
(Assignee)

Comment 13

5 years ago
(In reply to Mike Hommey [:glandium] from comment #12)
> Comment on attachment 829728 [details] [diff] [review] only use arc4random_addrandom if it exists
> Considering all the comments in this bug, why not just #define
> arc4random_addrandom(...) in openbsd's event-config.h?

we want the api to disappear, not to stay as a noop forever...
we've now removed evutil_secure_rng_add_bytes() from our libevent API port, since nothing uses it, and there was a big scary warning in event2/util.h about it, see https://github.com/libevent/libevent/blob/master/include/event2/util.h#L722

so the libevent function should disappear, as the sole caller of an api that is bad.
I'm talking about the possible patch for mozilla-central. Note bsd/event-config.h is *not* a libevent file. It's generated from a configure run (which means patching it is kind of dangerous, but less so it is documented).
(Assignee)

Comment 15

5 years ago
Created attachment 831776 [details] [diff] [review]
#ifndef OpenBSD evutil_secure_rng_add_bytes()

So, given that bsd/event-config.h is a generated file (well, actually it got made up in bug 864013) i'm not going to even dare touch it, and decided to go the simplest way instead: #ifndef'out evutil_secure_rng_add_bytes() on OpenBSD, since nothing uses it in-tree, and it breaks the build on OpenBSD if defined.

I think i should have all the bits for README.mozilla + the added patch.
Attachment #831776 - Flags: review?(joshmoz)
(Assignee)

Comment 16

5 years ago
josh ? can we move forward on this ?
Flags: needinfo?(joshmoz)

Updated

5 years ago
Attachment #831776 - Flags: review?(joshmoz) → review+

Updated

5 years ago
Flags: needinfo?(joshmoz)
https://hg.mozilla.org/mozilla-central/rev/c534f99dacf6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.