1821002 - Crash [@ mozilla::ipc::IProtocol::CanSend] through [@ mozilla::dom::PContentParent::SendClearFocus]

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P3)

Description

[Christian Holler (:decoder)]

In experimental IPC fuzzing, we found the following crash on mozilla-central revision f14ed3bab724+ (fuzzing-asan-nyx-opt build):

=================================================================
==1739==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000011 (pc 0x7fffcb255e01 bp 0x7ffffffdc550 sp 0x7ffffffdc530 T0)
==1739==The signal is caused by a READ memory access.
==1739==Hint: address points to the zero page.
    #0 0x7fffcb255e01 in mozilla::ipc::IProtocol::CanSend() const objdir-ff-aflpp/dist/include/mozilla/ipc/ProtocolUtils.h:226:45
    #1 0x7fffcd88adee in mozilla::ipc::IProtocol::ChannelSend(mozilla::UniquePtr<IPC::Message, mozilla::DefaultDelete<IPC::Message> >) ipc/glue/ProtocolUtils.cpp:481:7
    #2 0x7fffda9dee3c in mozilla::dom::PContentParent::SendClearFocus(mozilla::dom::MaybeDiscarded<mozilla::dom::BrowsingContext> const&) objdir-ff-aflpp/ipc/ipdl/PContentParent.cpp:5803:21
    #3 0x7fffda5ef61b in mozilla::dom::ContentParent::RecvClearFocus(mozilla::dom::MaybeDiscarded<mozilla::dom::BrowsingContext> const&) dom/ipc/ContentParent.cpp:7443:19
    #4 0x7fffdaa3ee1e in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) objdir-ff-aflpp/ipc/ipdl/PContentParent.cpp:16224:81
    [...]
    #31 0x5555557280cd in _start ??:0:0

The issue here is the same as in bug 1816687 and there are a few more places in PContentParent that look the same. I will replace all of them. There are some occurrences though where a cp is aquired and used without a null check where the origin of the cp looks different, so I am not touching that for now. Help/advice appreciated if these places should also be refactored or need null checks.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Jens Stutte [:jstutte]]

(In reply to Christian Holler (:decoder) from comment #0)

There are some occurrences though where a cp is aquired and used without a null check where the origin of the cp looks different, so I am not touching that for now. Help/advice appreciated if these places should also be refactored or need null checks.

Can you provide pointers to those you'd like to be helped with? Thanks!

Comment 4

[Christian Holler (:decoder)]

Attachment: Bug 1821002 - Add missing null checks for ContentParent. r?nika

Comment 5

[Christian Holler (:decoder)]

(In reply to Jens Stutte [:jstutte] from comment #3)

(In reply to Christian Holler (:decoder) from comment #0)

There are some occurrences though where a cp is aquired and used without a null check where the origin of the cp looks different, so I am not touching that for now. Help/advice appreciated if these places should also be refactored or need null checks.

Can you provide pointers to those you'd like to be helped with? Thanks!

One location I found immediately is

https://searchfox.org/mozilla-central/rev/416f31a51174620f04fc994d248b664b54517699/dom/ipc/ContentParent.cpp#7685-7701

I rechecked the others and the few I could find already check cp anyway, so we don't need to refactor.

Comment 6

[Jens Stutte [:jstutte]]

(In reply to Christian Holler (:decoder) from comment #5)

One location I found immediately is

https://searchfox.org/mozilla-central/rev/416f31a51174620f04fc994d248b664b54517699/dom/ipc/ContentParent.cpp#7685-7701

I rechecked the others and the few I could find already check cp anyway, so we don't need to refactor.

It looks as if we check some (hopefully all?) possible quirks in the input parameters and have the MOZ_RELEASE_ASSERT there in case. First of all I wonder why we do not just return IPC_FAIL here to crash the child process rather than crashing ourselves? In any case GetContentProcessById can always unexpectedly fail, so having null checks there seems advisable, too. I'd assume in that case we can just return IPC_OK (and maybe log a NS_WARNING), but I am probably not the right person to confirm/review this.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d8084f876dc
Add missing null checks for ContentParent. r=nika

Comment 8

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/1d8084f876dc

Comment 9

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

The patch landed in nightly and beta is affected.
:decoder, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox112 to wontfix.

For more information, please visit auto_nag documentation.