Skip to content

Commit

Permalink
[wasm] crypto deadlock fix (#73537)
Browse files Browse the repository at this point in the history
* more state locking and sanity
* renamed the subtle-crypto.ts file, because that's not a worker
Co-authored-by: Ankit Jain <radical@gmail.com>
  • Loading branch information
pavelsavara authored Aug 11, 2022
1 parent 2594ec1 commit bb97114
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 93 deletions.
2 changes: 1 addition & 1 deletion src/mono/wasm/runtime/exports-linker.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import MonoWasmThreads from "consts:monoWasmThreads";
import { dotnet_browser_can_use_subtle_crypto_impl, dotnet_browser_simple_digest_hash, dotnet_browser_sign, dotnet_browser_encrypt_decrypt, dotnet_browser_derive_bits } from "./crypto-worker";
import { dotnet_browser_can_use_subtle_crypto_impl, dotnet_browser_simple_digest_hash, dotnet_browser_sign, dotnet_browser_encrypt_decrypt, dotnet_browser_derive_bits } from "./subtle-crypto";
import { mono_wasm_fire_debugger_agent_message, mono_wasm_debugger_log, mono_wasm_add_dbg_command_received, mono_wasm_set_entrypoint_breakpoint } from "./debug";
import { mono_wasm_release_cs_owned_object } from "./gc-handles";
import { mono_wasm_load_icu_data, mono_wasm_get_icudt_name } from "./icu";
Expand Down
2 changes: 1 addition & 1 deletion src/mono/wasm/runtime/startup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { mono_wasm_init_aot_profiler, mono_wasm_init_coverage_profiler } from ".
import { mono_on_abort, set_exit_code } from "./run";
import { initialize_marshalers_to_cs } from "./marshal-to-cs";
import { initialize_marshalers_to_js } from "./marshal-to-js";
import { init_crypto } from "./crypto-worker";
import { init_crypto } from "./subtle-crypto";
import { init_polyfills_async } from "./polyfills";
import * as pthreads_worker from "./pthreads/worker";
import { createPromiseController } from "./promise-controller";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,22 +207,13 @@ class LibraryChannel {

public send_msg(msg: string): string {
try {
let state = Atomics.load(this.comm, this.STATE_IDX);
// FIXME: this console write is possibly serializing the access and prevents a deadlock
if (state !== this.STATE_IDLE) console.debug(`MONO_WASM_ENCRYPT_DECRYPT: send_msg, waiting for idle now, ${state}`);
state = this.wait_for_state(pstate => pstate == this.STATE_IDLE, "waiting");

this.wait_for_state_change_to(pstate => pstate == this.STATE_IDLE, "waiting");
this.send_request(msg);
return this.read_response();
} catch (err) {
this.reset(LibraryChannel._stringify_err(err));
this.reset(LibraryChannel.stringify_err(err));
throw err;
}
finally {
const state = Atomics.load(this.comm, this.STATE_IDX);
// FIXME: this console write is possibly serializing the access and prevents a deadlock
if (state !== this.STATE_IDLE) console.debug(`MONO_WASM_ENCRYPT_DECRYPT: state at end of send_msg: ${state}`);
}
}

public shutdown(): void {
Expand All @@ -231,9 +222,11 @@ class LibraryChannel {
if (state !== this.STATE_IDLE)
throw new Error(`OWNER: Invalid sync communication channel state: ${state}`);

this.using_lock(() => {
Atomics.store(this.comm, this.MSG_SIZE_IDX, 0);
this.change_state_locked(this.STATE_SHUTDOWN);
});
// Notify webworker
Atomics.store(this.comm, this.MSG_SIZE_IDX, 0);
this._change_state_locked(this.STATE_SHUTDOWN);
Atomics.notify(this.comm, this.STATE_IDX);
}

Expand All @@ -248,8 +241,11 @@ class LibraryChannel {
return;
}

Atomics.store(this.comm, this.MSG_SIZE_IDX, 0);
this._change_state_locked(this.STATE_RESET);
this.using_lock(() => {
Atomics.store(this.comm, this.MSG_SIZE_IDX, 0);
this.change_state_locked(this.STATE_RESET);
});
// Notify webworker
Atomics.notify(this.comm, this.STATE_IDX);
}

Expand All @@ -259,9 +255,7 @@ class LibraryChannel {
let msg_written = 0;

for (; ;) {
this.acquire_lock();

try {
this.using_lock(() => {
// Write the message and return how much was written.
const wrote = this.write_to_msg(msg, msg_written, msg_len);
msg_written += wrote;
Expand All @@ -272,19 +266,20 @@ class LibraryChannel {
// Indicate if this was the whole message or part of it.
state = msg_written === msg_len ? this.STATE_REQ : this.STATE_REQ_P;

// Notify webworker
this._change_state_locked(state);
} finally {
this.release_lock();
}

this.change_state_locked(state);
});
// Notify webworker
Atomics.notify(this.comm, this.STATE_IDX);

// The send message is complete.
if (state === this.STATE_REQ)
if (state === this.STATE_REQ) {
break;
}
else if (state !== this.STATE_REQ_P) {
throw new Error(`Unexpected state ${state}`);
}

this.wait_for_state(state => state == this.STATE_AWAIT, "send_request");
this.wait_for_state_change_to(state => state == this.STATE_AWAIT, "send_request");
}
}

Expand All @@ -302,56 +297,63 @@ class LibraryChannel {
private read_response(): string {
let response = "";
for (; ;) {
const state = this.wait_for_state(state => state == this.STATE_RESP || state == this.STATE_RESP_P, "read_response");
this.acquire_lock();

try {
this.wait_for_state_change_to(state => state == this.STATE_RESP || state == this.STATE_RESP_P, "read_response");
const done = this.using_lock(() => {
const size_to_read = Atomics.load(this.comm, this.MSG_SIZE_IDX);

// Append the latest part of the message.
response += this.read_from_msg(0, size_to_read);

// The response is complete.
const state = Atomics.load(this.comm, this.STATE_IDX);
if (state === this.STATE_RESP) {
Atomics.store(this.comm, this.MSG_SIZE_IDX, 0);
break;
return true;
} else if (state !== this.STATE_RESP_P) {
throw new Error(`Unexpected state ${state}`);
}

// Reset the size and transition to await state.
Atomics.store(this.comm, this.MSG_SIZE_IDX, 0);
this._change_state_locked(this.STATE_AWAIT);
} finally {
this.release_lock();
}
this.change_state_locked(this.STATE_AWAIT);
return false;
});

// Notify webworker
Atomics.notify(this.comm, this.STATE_IDX);

if (done) {
break;
}
}

// Reset the communication channel's state and let the
// webworker know we are done.
this._change_state_locked(this.STATE_IDLE);
this.using_lock(() => {
this.change_state_locked(this.STATE_IDLE);
});
Atomics.notify(this.comm, this.STATE_IDX);

return response;
}

private _change_state_locked(newState: number): void {
private change_state_locked(newState: number): void {
Atomics.store(this.comm, this.STATE_IDX, newState);
}

private wait_for_state(is_ready: (state: number) => boolean, msg: string): number {
private wait_for_state_change_to(is_ready: (state: number) => boolean, msg: string): void {
// Wait for webworker
// - Atomics.wait() is not permissible on the main thread.
for (; ;) {
const lock_state = Atomics.load(this.comm, this.LOCK_IDX);
if (lock_state !== this.LOCK_UNLOCKED)
continue;

const state = Atomics.load(this.comm, this.STATE_IDX);
if (state == this.STATE_REQ_FAILED)
throw new OperationFailedError(`Worker failed during ${msg} with state=${state}`);
const done = this.using_lock(() => {
const state = Atomics.load(this.comm, this.STATE_IDX);
if (state == this.STATE_REQ_FAILED)
throw new OperationFailedError(`Worker failed during ${msg} with state=${state}`);

if (is_ready(state))
return state;
if (is_ready(state))
return true;
});
if (done) return;
}
}

Expand All @@ -361,6 +363,15 @@ class LibraryChannel {
return String.fromCharCode.apply(null, slicedMessage);
}

private using_lock(callback: Function) {
try {
this.acquire_lock();
return callback();
} finally {
this.release_lock();
}
}

private acquire_lock() {
for (; ;) {
const lock_state = Atomics.compareExchange(this.comm, this.LOCK_IDX, this.LOCK_UNLOCKED, this.LOCK_OWNED);
Expand All @@ -381,7 +392,7 @@ class LibraryChannel {
}
}

private static _stringify_err(err: any) {
private static stringify_err(err: any) {
return (err instanceof Error && err.stack !== undefined) ? err.stack : err;
}

Expand Down
Loading

0 comments on commit bb97114

Please sign in to comment.