897723 - MappableSeekableZStream::ensure can deadlock when profiler is on

Status: RESOLVED FIXED

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Block signals to avoid mutex deadlock in MappableSeekableZStream::ensure

MappableSeekableZStream::ensure can deadlock in the following scenario,

1) On-demand decompression causes SIGSEGV
2) SIGSEGV handler calls MappableSeekableZStream::ensure
3) MappableSeekableZStream::ensure locks its mutex
4) Profiler causes SIGPROF
5) SIGPROF handler is not available, causing SIGSEGV again
6) SIGSEGV handler calls MappableSeekableZStream::ensure again
7) MappableSeekableZStream::ensure deadlocks on already-owned mutex

This can conceivably happen with other signals, but I don't know if we do use other signals.

FWIW, I verified that the attached patch fixes the issue by blocking signals during the lock.

Comment 1

[Mike Hommey [:glandium]]

Comment on attachment 780648 [details] [diff] [review]
Block signals to avoid mutex deadlock in MappableSeekableZStream::ensure

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

Two things I'd like you to check:
- what happens if a segv happens while the lock is held (you can test what happens if you make MappableSeekableZStream::ensure MOZ_CRASH randomly).
- how well the profiler responds to its SIGPROF being delayed (probably fine).

Comment 2

[Mike Hommey [:glandium]]

(In reply to Jim Chen [:jchen :nchen] from comment #0)
> Created attachment 780648 [details] [diff] [review]
> Block signals to avoid mutex deadlock in MappableSeekableZStream::ensure
> 
> MappableSeekableZStream::ensure can deadlock in the following scenario,
> 
> 1) On-demand decompression causes SIGSEGV
> 2) SIGSEGV handler calls MappableSeekableZStream::ensure
> 3) MappableSeekableZStream::ensure locks its mutex
> 4) Profiler causes SIGPROF
> 5) SIGPROF handler is not available, causing SIGSEGV again
> 6) SIGSEGV handler calls MappableSeekableZStream::ensure again
> 7) MappableSeekableZStream::ensure deadlocks on already-owned mutex

Note there needs something else for that to be triggered: the code setting the SIGPROF handler needs to be in a different page than the handler code itself.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Mike Hommey [:glandium] from comment #2)
> 
> Note there needs something else for that to be triggered: the code setting
> the SIGPROF handler needs to be in a different page than the handler code
> itself.

Right. Unfortunately the SIGPROF handler calls code from a few different modules, so the deadlock is quite likely to happen.

(In reply to Mike Hommey [:glandium] from comment #1)
> Comment on attachment 780648 [details] [diff] [review]
> Block signals to avoid mutex deadlock in MappableSeekableZStream::ensure
> 
> Review of attachment 780648 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Two things I'd like you to check:
> - what happens if a segv happens while the lock is held (you can test what
> happens if you make MappableSeekableZStream::ensure MOZ_CRASH randomly).

Seems like the process gets killed immediately without further action.

> - how well the profiler responds to its SIGPROF being delayed (probably
> fine).

I think BenWa is best to answer that.

Comment 4

[Mike Hommey [:glandium]]

I think I have a better idea.

Comment 5

[Mike Hommey [:glandium]]

Also, I wonder if that couldn't be the cause of bug 886736, not with the profiler, but with something else on the system that would send signals.

Comment 6

[Mike Hommey [:glandium]]

Attachment: Allow faulty.lib's on-demand decompression to be reentrant

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 780883 [details] [diff] [review]
Allow faulty.lib's on-demand decompression to be reentrant

Jim, can you check this fixes it for you? I only tested on desktop.

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 780883 [details] [diff] [review]
Allow faulty.lib's on-demand decompression to be reentrant

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

Yep, works fine.

Comment 9

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4716f738ef0a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4716f738ef0a

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 780883 [details] [diff] [review]
Allow faulty.lib's on-demand decompression to be reentrant

[Approval Request Comment]
Bug caused by bug 848764
User impact if declined: Enabling the profiler freezes Firefox for Android
Testing completed (on m-c, etc.): Landed on m-c a few days ago. Tested by jchen.
Risk to taking this patch (and alternatives if risky): It's possible this could lead to weird problems on some systems, but it shouldn't be more risky than the whole on-demand decompression feature (see bug 886736 for example). On-demand decompression can be disabled per-device, per-system-version, or globally, if it needs to be. This patch can also easily be backed out, too (there won't be any landing that will make that difficult)
String or IDL/UUID changes made by this patch: None

Comment 12

[bhavana bajaj [:bajaj]]

(In reply to Mike Hommey [:glandium] from comment #11)
> Comment on attachment 780883 [details] [diff] [review]
> Allow faulty.lib's on-demand decompression to be reentrant
> 
> [Approval Request Comment]
> Bug caused by bug 848764
> User impact if declined: Enabling the profiler freezes Firefox for Android
> Testing completed (on m-c, etc.): Landed on m-c a few days ago. Tested by
> jchen.
> Risk to taking this patch (and alternatives if risky): It's possible this
> could lead to weird problems on some systems,

Do you have testcases in mind that QA can help with ? Also can you please make sure to keep an eye on new regressions if any by this landing ?

> but it shouldn't be more risky
> than the whole on-demand decompression feature (see bug 886736 for example).
> On-demand decompression can be disabled per-device, per-system-version, or
> globally, if it needs to be. This patch can also easily be backed out, too
> (there won't be any landing that will make that difficult)
> String or IDL/UUID changes made by this patch: None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/30132c3fe38a

Comment 14

[Mike Hommey [:glandium]]

(In reply to bhavana bajaj [:bajaj] from comment #12)
> Do you have testcases in mind that QA can help with ?

The best i can think of is stress testing starting fennec and display web pages on plenty of different devices.

> Also can you please make sure to keep an eye on new regressions if any by this landing ?

Sure