The WebContent process should not process incoming IPC while waiting for a sync IPC reply

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

The WebContent process should not process incoming IPC while waiting for a sync IPC reply.

[Chris Dumez]

Created attachment 331124 [details]
WIP Patch

[Alexey Proskuryakov]

Wouldn’t this cause unresolvable deadlocks?

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #2)
> Wouldn’t this cause unresolvable deadlocks?

It could but as you can see tests are passing. I also tried API tests locally.
I do not agree that it would be unresolvable though. If somebody relies on a Sync IPC 1from the WebProcess to another process and then that other process does another sync IPC 2 to the WebProcess, then we can fix it by refactoring the code so that IPC 2 is not needed (or if we know that it is a safe time, we can set the flag on the sendSync() to allow it).

[Chris Dumez]

Created attachment 331162 [details]
WIP Patch

[Ryosuke Niwa]

(In reply to Alexey Proskuryakov from comment #2)
> Wouldn’t this cause unresolvable deadlocks?

The key is that we still allow UI process to process sync messages while it awaits for a  sync reply. It means that if WebContent process needs a sync reply from UI process's sync message, then UIProcess can respond to that.

Note that it is possible to introduce a deadlock in this situation if WebContent process sends a sync message and UI process needs to send WebContent process another sync message before answering that sync message. Hopefully, we don't have such a message, and Chris' testing shows that we don't seem to have such a message.

[Chris Dumez]

Created attachment 331206 [details]
WIP Patch

[EWS Watchlist]

Comment on attachment 331206 [details]
WIP Patch

Attachment 331206 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6050841

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 331212 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 331215 [details]
WIP Patch

[EWS Watchlist]

Attachment 331215 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Plugins/PluginProcessConnection.cpp:53:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 331220 [details]
WIP Patch

[Chris Dumez]

Created attachment 331224 [details]
WIP Patch

[Chris Dumez]

Created attachment 331226 [details]
WIP Patch

[Chris Dumez]

Created attachment 331229 [details]
WIP Patch

[Chris Dumez]

Created attachment 331231 [details]
WIP Patch

[Chris Dumez]

Created attachment 331240 [details]
WIP Patch

[Chris Dumez]

Created attachment 331242 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 331242 [details]
Patch

Layout tests are useless to verify this change, as we always use the UseFullySynchronousModeForTesting mode there. Also, we don't have high fidelity tests for inline input.

> Note that it is possible to introduce a deadlock in this situation if WebContent process sends a sync message and UI process needs to send WebContent process another sync message before answering that sync message. Hopefully, we don't have such a message, and Chris' testing shows that we don't seem to have such a message.

UI processes sends sync messages at arbitrary times (text input client calls is a notable case of sync messages without a timeout). So I don't see why you hope that there aren't such messages.

This approach continues to look wrong to me. Furthermore, I do not agree that this behavior is worth being concerned about. It is not unexpected that things can happen while waiting for a sync response. In fact, it is exactly the same behavior that we have in legacy WebKit - delegate callouts may and do reenter.

[Ryosuke Niwa]

(In reply to Alexey Proskuryakov from comment #18)
> Comment on attachment 331242 [details]
> Patch
> 
> Layout tests are useless to verify this change, as we always use the
> UseFullySynchronousModeForTesting mode there. Also, we don't have high
> fidelity tests for inline input.
> 
> > Note that it is possible to introduce a deadlock in this situation if WebContent process sends a sync message and UI process needs to send WebContent process another sync message before answering that sync message. Hopefully, we don't have such a message, and Chris' testing shows that we don't seem to have such a message.
> 
> UI processes sends sync messages at arbitrary times (text input client calls
> is a notable case of sync messages without a timeout). So I don't see why
> you hope that there aren't such messages.

That's okay. UI processes sending a sync message to WebContent process would always work unless it's doing so while responding to a sync message from WebContent process.

> This approach continues to look wrong to me. Furthermore, I do not agree
> that this behavior is worth being concerned about. It is not unexpected that
> things can happen while waiting for a sync response. In fact, it is exactly
> the same behavior that we have in legacy WebKit - delegate callouts may and
> do reenter.

The problem is that many sync IPC messages are sent from WebContent process while it's not save to execute arbitrary scripts. We have a security bug whenever we happen to process sync messages while waiting for the reply.

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #18)
> Comment on attachment 331242 [details]
> Patch
> 
> Layout tests are useless to verify this change, as we always use the
> UseFullySynchronousModeForTesting mode there. Also, we don't have high
> fidelity tests for inline input.
> 
> > Note that it is possible to introduce a deadlock in this situation if WebContent process sends a sync message and UI process needs to send WebContent process another sync message before answering that sync message. Hopefully, we don't have such a message, and Chris' testing shows that we don't seem to have such a message.
> 
> UI processes sends sync messages at arbitrary times (text input client calls
> is a notable case of sync messages without a timeout). So I don't see why
> you hope that there aren't such messages.

Yes, the UIProcess sends sync messages to the WebProcess at arbitrary times, and this is fine. The only thing that is not fine is that the WebProcess would potentially process those incoming sync IPC messages while waiting for the reply to another Sync message the WebProcess sent. This keeps being a source of crashes and security bug, which is why we are trying to change this behavior.

Note that these sync IPC messages from the UIProcess will still be processed. However, they will not be processed while the WebProcess is waiting for a sync reply. It will wait for that sync reply to be received before processing other incoming IPC.

Yes, there is a case where this could cause deadlocks in the specific scenario mentioned earlier. So far, we do not have evidence this is happening though. However, we have a lot of evidence of crashes happening due to the current IPC behavior. If we do cause a deadlock (for something not covered by tests), we can either refactor the code or whitelist the message in question if it happens at a safe time (I.e script is allowed to run).

> 
> This approach continues to look wrong to me. Furthermore, I do not agree
> that this behavior is worth being concerned about. It is not unexpected that
> things can happen while waiting for a sync response. In fact, it is exactly
> the same behavior that we have in legacy WebKit - delegate callouts may and
> do reenter.

[Alexey Proskuryakov]

> UI processes sending a sync message to WebContent process would always work unless it's doing so while responding to a sync message from WebContent process.

I see. Maybe we don't have such cases between WebContent process and other processes indeed. But I don't know about all sync messages that UI process sends.

> The problem is that many sync IPC messages are sent from WebContent process while it's not save to execute arbitrary scripts. We have a security bug whenever we happen to process sync messages while waiting for the reply.

Sure, but how many are cases where the same issue won't happen in WebKit1, to with bundle clients? It's normal for a delegate to perform unexpected work, and swiping one specific case under the carpet seems like a poor defense.

[Ryosuke Niwa]

(In reply to Alexey Proskuryakov from comment #21)
>
> > The problem is that many sync IPC messages are sent from WebContent process while it's not save to execute arbitrary scripts. We have a security bug whenever we happen to process sync messages while waiting for the reply.
> 
> Sure, but how many are cases where the same issue won't happen in WebKit1,
> to with bundle clients? It's normal for a delegate to perform unexpected
> work, and swiping one specific case under the carpet seems like a poor
> defense.

Indeed, delegate callbacks in WK1 and bundle callbacks are an issue. In fact, we had to disable my security assertion in WK1 because many embedding apps are running scripts when it's unsafe to do so. I don't think there is a practical way to fix that in WK1 unfortunately because we simply can't make running arbitrary scripts in the middle of layout, etc... safe. Since injected bundle's callbacks aren't a part of public API (they're only usable by the third party apps), this patch still materially improves the security of WK2 clients including but not limited to Safari.

As is, we have dozens upon dozens of crashes and security bugs due to the current IPC behavior, and there is no practical way to address all those issues by moving IPC outside the unsafe code paths because there are simply too many of them.

[Alexey Proskuryakov]

Comment on attachment 331242 [details]
Patch

I'm not comfortable enough with this patch to r+, but I don't feel comfortable blocking it either. Sorry for the muddled message.

[Ryosuke Niwa]

Comment on attachment 331242 [details]
Patch

r=me I think this change makes sense. We'll do more manual testing like typing fast on iPads per Alexey's suggestion.

[WebKit Commit Bot]

Comment on attachment 331242 [details]
Patch

Clearing flags on attachment: 331242

Committed r227216: <https://trac.webkit.org/changeset/227216>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36663395>