Tyson Smith [:tsmith] Reporter
	Description • 7 years ago

Found in mozilla-central changeset: 402372:3df7961bad2c. Built with -fsanitize=nonnull-attribute

Sorry I don't have STR ATM. I think it was just regular browsing that triggered this.

/ipc/chromium/src/chrome/common/ipc_channel_posix.cc:428:47: runtime error: null pointer passed as argument 2, which is declared to never be null
/usr/include/string.h:43:28: note: nonnull attribute specified here
    #0 0x7f61df1d298a in IPC::Channel::ChannelImpl::ProcessIncomingMessages() /ipc/chromium/src/chrome/common/ipc_channel_posix.cc:428:7
    #1 0x7f61df1d478c in IPC::Channel::ChannelImpl::OnFileCanReadWithoutBlocking(int) /ipc/chromium/src/chrome/common/ipc_channel_posix.cc:800:10
    #2 0x7f61df27ab6f in event_persist_closure /ipc/chromium/src/third_party/libevent/event.c:1580:9
    #3 0x7f61df278f2e in event_process_active_single_queue /ipc/chromium/src/third_party/libevent/event.c:1639:4
    #4 0x7f61df23eb6b in event_process_active /ipc/chromium/src/third_party/libevent/event.c
    #5 0x7f61df23b86f in event_base_loop /ipc/chromium/src/third_party/libevent/event.c:1961:12
    #6 0x7f61df16b477 in base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) /ipc/chromium/src/base/message_pump_libevent.cc:373:7
    #7 0x7f61df167090 in MessageLoop::Run() /ipc/chromium/src/base/message_loop.cc:299:3
    #8 0x7f61df1a91da in base::Thread::ThreadMain() /ipc/chromium/src/base/thread.cc:181:16
    #9 0x7f61df172209 in ThreadFunc(void*) /ipc/chromium/src/base/platform_thread_posix.cc:38:13
    #10 0x7f62170b17fb in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x77fb)
    #11 0x7f62160dfb5e in clone /build/glibc-itYbWN/glibc-2.26/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:95

	Jed Davis [:jld] (on leave until 08-31) Assignee
	Comment 1 • 7 years ago

From the code it looks like this might be a case of memcpy(_, nullptr, 0), which in principle should be a no-op, but might actually be undefined behavior according to the C spec.  (There are a few weird cases like this, and there may be people who have them memorized, but I am not one of them.)

If that's all it is, it should be simple enough to skip the memcpy if num_wire_fds is 0.

	Jed Davis [:jld] (on leave until 08-31) Assignee
	Comment 2 • 6 years ago

This is in fact UB — see https://www.imperialviolet.org/2016/06/26/nonnull.html

I don't see any obvious hazards as far as null check removal, but the compiler *is* allowed to do whatever it wants when this happens.

	Christian Stadelmann
	Comment 3 • 6 years ago

I think this causes a lot of crashes in Fedora's firefox 60 builds: https://bugzilla.redhat.com/show_bug.cgi?id=1577277

Crash reporting has a few related stats: https://crash-stats.mozilla.com/signature/?product=Firefox&signature=firefox%400x114b2

	Jed Davis [:jld] (on leave until 08-31) Assignee
	Comment 4 • 6 years ago

(In reply to Christian Stadelmann from comment #3)
> I think this causes a lot of crashes in Fedora's firefox 60 builds:
> https://bugzilla.redhat.com/show_bug.cgi?id=1577277

Yes and no.  That *is* an instance of undefined behavior in the same line of code, but it's a different one than UBSan found: evaluating the first argument indexes past the end of a std::vector.  Unlike with C arrays, taking a pointer to the element immediately after the end isn't allowed, and libstdc++ has optional assertions that enforce this.

(The stack trace in https://bugzilla.redhat.com/show_bug.cgi?id=1579939 was helpful here — Breakpad doesn't handle inlining well, doesn't have access to the strings in the libstdc++ assertion failure, and for some reason it doesn't have debug info for the firefox executables in Fedora's builds even though it does have info for libxul.)

The fix is trivial, so I'll pick up this bug.

	Jed Davis [:jld] (on leave until 08-31) Assignee
	Comment 5 • 6 years ago

For reference, this was fixed in Chromium in September 2009 (mixed in with several other changes, and doesn't seem to be explicitly mentioned in the bug tracker or code review system): https://github.com/chromium/chromium/commit/baf556aa12ddfb301f2a54f5c741505b55bad18c#diff-757d760e1011ed44704430032eb22f5bR538

	Jed Davis [:jld] (on leave until 08-31) Assignee
	Comment 6 • 6 years ago

Also the corresponding code in Chromium was rewritten in 2012 to use std::deque (and not memcpy anything): https://github.com/chromium/chromium/commit/334f302db0310f47d20eb532cb0bae401a0eddbe → https://codereview.chromium.org/9533002

(And in 2016 it was completely deleted in favor of Mojo: https://crbug.com/659448 )

	Christian Stadelmann
	Comment 7 • 6 years ago

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #4)
> 
> (The stack trace in https://bugzilla.redhat.com/show_bug.cgi?id=1579939 was
> helpful here — Breakpad doesn't handle inlining well, doesn't have access to
> the strings in the libstdc++ assertion failure, and for some reason it
> doesn't have debug info for the firefox executables in Fedora's builds even
> though it does have info for libxul.)
> 
> The fix is trivial, so I'll pick up this bug.

Wow, nice, thank you!
As this affects Firefox 60, will the fix land in a 60.0.x release?

	Nathan Froyd [:froydnj]
	Comment 9 • 6 years ago
mozreview-review

Comment on attachment 8979425 [details]
Bug 1436242 - Avoid undefined behavior in IPC fd-passing code.

https://reviewboard.mozilla.org/r/245604/#review251836

Fun.

	Jed Davis [:jld] (on leave until 08-31) Assignee
	Comment 10 • 6 years ago

(In reply to Christian Stadelmann from comment #7)
> As this affects Firefox 60, will the fix land in a 60.0.x release?

I'll request uplift to release; in the meantime, distributions can always apply the patch to their own builds if they're affected.

	Pulsebot
	Comment 11 • 6 years ago

Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6bb3adfa15c6
Avoid undefined behavior in IPC fd-passing code.  r=froydnj

	Dorel Luca [:dluca]
	Comment 12 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6bb3adfa15c6

	Jed Davis [:jld] (on leave until 08-31) Assignee
	Comment 14 • 6 years ago

Needinfo'ing myself to request uplift once this has had a few days on mozilla-central.

I see that this has been marked wontfix for 60.0, and I suppose that makes sense because I don't think this affects Mozilla's own builds, but that means Fedora will need to apply the patch locally.

	Jed Davis [:jld] (on leave until 08-31) Assignee
	Comment 15 • 6 years ago

On second thought I'm not going to request uplift; this doesn't seem to affect Mozilla's builds, so I don't think release management would accept it for beta.