931354 - No arc4random_addrandom symbol defined anymore

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Landry Breuil (:gaston)]

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.

Comment 1

[Landry Breuil (:gaston)]

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

Comment 2

[Landry Breuil (:gaston)]

Ben, Chris, any hindsight ?

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 4

[Landry Breuil (:gaston)]

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

[Josh Aas]

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

Comment 6

[Landry Breuil (:gaston)]

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 ?

Comment 7

[Landry Breuil (:gaston)]

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

Comment 8

[Landry Breuil (:gaston)]

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

[Josh Aas]

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.

Comment 10

[Josh Aas]

Actually, perhaps it is better to ifdef the entire function and callsites so nobody accidentally calls a function that does nothing.

Comment 11

[Landry Breuil (:gaston)]

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 12

[Mike Hommey [:glandium]]

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?

Comment 13

[Landry Breuil (:gaston)]

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

Comment 14

[Mike Hommey [:glandium]]

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

Comment 15

[Landry Breuil (:gaston)]

Attachment: #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.

Comment 16

[Landry Breuil (:gaston)]

josh ? can we move forward on this ?

Comment 17

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c534f99dacf6

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c534f99dacf6