1929482 - Crash in [@ mozilla::ProfileBufferEntryWriter::WriteBytes]

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P2)

Description

[Paul Bone [:pbone]]

Crash report: https://crash-stats.mozilla.org/report/index/ce036433-8863-4266-ae58-204100241030

Reason:

EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE

Top 10 frames:

0  libsystem_platform.dylib  _platform_memmove
1  XUL  mozilla::ProfileBufferEntryWriter::WriteBytes(void const*, unsigned int)  mozglue/baseprofiler/public/ProfileBufferEntrySerialization.h:517
2  XUL  mozilla::Variant<long long, bool, double, mozilla::ProfilerStringView<char> >...  mfbt/Variant.h:840
2  XUL  mozilla::ProfileBufferEntryWriter::Serializer<mozilla::Variant<long long, boo...  mozglue/baseprofiler/public/ProfileBufferEntrySerialization.h:1132
2  XUL  mozilla::ProfileBufferEntryWriter::WriteObject<mozilla::Variant<long long, bo...  mozglue/baseprofiler/public/ProfileBufferEntrySerialization.h:559
2  XUL  mozilla::ProfileBufferEntryWriter::Serializer<(anonymous namespace)::TraceOpt...  tools/profiler/core/MicroGeckoProfiler.cpp:158
2  XUL  mozilla::ProfileBufferEntryWriter::WriteObject<(anonymous namespace)::TraceOp...  mozglue/baseprofiler/public/ProfileBufferEntrySerialization.h:559
3  XUL  mozilla::ProfileBufferEntryWriter::Serializer<std::__1::tuple<(anonymous name...  mozglue/baseprofiler/public/ProfileBufferEntrySerialization.h:978
3  XUL  mozilla::ProfileBufferEntryWriter::Serializer<std::__1::tuple<(anonymous name...  mozglue/baseprofiler/public/ProfileBufferEntrySerialization.h:990
3  XUL  mozilla::ProfileBufferEntryWriter::WriteObject<std::__1::tuple<(anonymous nam...  mozglue/baseprofiler/public/ProfileBufferEntrySerialization.h:559

This is a use-after-free detected by PHC, the crashing address is 0x0000000105697ff8 and the object is allocated at 0x105697ff0

Free stack:

#0    uprofiler_simple_event_marker_internal(char const*, char, char, int, char const**, unsigned char const*, unsigned long long const*, bool, void*) (XUL)
#1    uprofiler_simple_event_marker (XUL)
#2    webrtc::VideoStreamEncoder::EncodeVideoFrame(webrtc::VideoFrame const&, long long) (XUL)
#3    webrtc::VideoStreamEncoder::MaybeEncodeVideoFrame(webrtc::VideoFrame const&, long long) (XUL)
#4    webrtc::VideoStreamEncoder::OnFrame(webrtc::Timestamp, bool, webrtc::VideoFrame const&) (XUL)
#5    absl::internal_any_invocable::RemoteInvoker<false, void, webrtc::(anonymous namespace)::FrameCadenceAdapterImpl::OnFrame(webrtc::VideoFrame const&)::$_0&&, >(absl::internal_any_invocable::TypeErasedState*, absl::internal_any_invocable::ForwardedParameter<>::type) (XUL)
#6    mozilla::detail::RunnableFunction<mozilla::TaskQueueWrapper<(mozilla::DeletionPolicy)0>::CreateTaskRunner(absl::AnyInvocable<void &&()>)::{lambda()#1}>::Run() (XUL)
#7    mozilla::TaskQueue::Runner::Run() (XUL)
#8    nsThreadPool::Run() (XUL)
#9    NS_ProcessNextEvent(nsIThread*, bool) (XUL)
#10    mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (XUL)
#11    MessageLoop::Run() (XUL)
#12    nsThread::ThreadFunc(void*) (XUL)
#13    _pt_root (libnss3.dylib)
#14    _pthread_start (libsystem_pthread.dylib)

Alloc stack:

#0    nsTSubstring<char>::Assign(char const*, unsigned long, std::nothrow_t const&) (XUL)
#1    nsTSubstring<char>::Assign(char const*, unsigned long) (XUL)
#2    uprofiler_simple_event_marker_internal(char const*, char, char, int, char const**, unsigned char const*, unsigned long long const*, bool, void*) (XUL)
#3    uprofiler_simple_event_marker (XUL)
#4    webrtc::VideoStreamEncoder::EncodeVideoFrame(webrtc::VideoFrame const&, long long) (XUL)
#5    webrtc::VideoStreamEncoder::MaybeEncodeVideoFrame(webrtc::VideoFrame const&, long long) (XUL)
#6    webrtc::VideoStreamEncoder::OnFrame(webrtc::Timestamp, bool, webrtc::VideoFrame const&) (XUL)
#7    absl::internal_any_invocable::RemoteInvoker<false, void, webrtc::(anonymous namespace)::FrameCadenceAdapterImpl::OnFrame(webrtc::VideoFrame const&)::$_0&&, >(absl::internal_any_invocable::TypeErasedState*, absl::internal_any_invocable::ForwardedParameter<>::type) (XUL)
#8    mozilla::detail::RunnableFunction<mozilla::TaskQueueWrapper<(mozilla::DeletionPolicy)0>::CreateTaskRunner(absl::AnyInvocable<void &&()>)::{lambda()#1}>::Run() (XUL)
#9    mozilla::TaskQueue::Runner::Run() (XUL)
#10    nsThreadPool::Run() (XUL)
#11    NS_ProcessNextEvent(nsIThread*, bool) (XUL)
#12    mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (XUL)
#13    MessageLoop::Run() (XUL)
#14    nsThread::ThreadFunc(void*) (XUL)
#15    _pt_root (libnss3.dylib)

Comment 1

[Andrew McCreight [:mccr8]]

I'll mark this sec-moderate as it presumably requires the profiler to be running.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:canova, could you have a look please?

For more information, please visit BugBot documentation.

Comment 3

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Hey Paul, do you remember how this was happening, or do you remember any way to reproduce this issue locally?

It looks like it's happening while reading the buffer contents.

My first instinct is that probably we are failing to copy the full strings inside uprofiler_simple_event_marker_internal while passing to profiler_add_marker. Trying to pass them as reference and/or ProfilerString8View is not copying the whole string to the buffer and instead puts a pointer only. And we try to read that later. Or it can be because of how we serialize the std::tuple / TraceOption in the buffer. As I don't remember using these things heavily inside the markers before, so there could be some issues there.

From a quick look I can't notice something concerning yet, but I will look into it more. It would help a lot if I can reproduce it locally or can get a pernosco session somehow though.

Comment 4

[Paul Bone [:pbone]]

Sorry. As far as I know this is not happening in CI where we can get a pernosco session. it's happening in the wild and we're getting crash reports.

Comment 6

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

This was one of my items in my TODO list and finally got around to look at it again. The bug was actually fairly obvious after looking with a fresh eye...

The problematic code is here: https://searchfox.org/mozilla-central/rev/c329cccde3e1e6f5d71d87c62b3db7e9f5fecb3c/tools/profiler/core/MicroGeckoProfiler.cpp#231-233,241-243,247-248

What happens is that:

1. We create a temporary string with nsPrintfCString.
2. Pass it to ProfilerString8View, which gets a reference to the string.
3. Then the temporary string gets freed at the end of that line.
4. And now ProfilerString8View holds a dangling pointer.

Comment 7

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: (secure)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/9070ee73eca3
https://hg.mozilla.org/integration/autoland/rev/43d897cc8e75
Make sure that strings live until serialization inside uprofiler_simple_event_marker_internal r=padenot

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/43d897cc8e75

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR140 approval when you get a chance.

Comment 11

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D257403

Comment 12

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: Intermittent crash when the gecko profiler is running
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: -
• Risk associated with taking this patch: Low
• Explanation of risk level: It's very simple with no behavior change other than the crash fix
• String changes made/needed: -
• Is Android affected?: yes

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/1a374444a167