From b9053b714c3405103dbdcfa7c375423071ce17ce Mon Sep 17 00:00:00 2001 From: Antony Agrios Date: Fri, 12 May 2023 18:16:29 +0300 Subject: [PATCH 01/10] Refactor: Simplify handleRequest Promise handling --- packages/php-wasm/universal/src/lib/base-php.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/php-wasm/universal/src/lib/base-php.ts b/packages/php-wasm/universal/src/lib/base-php.ts index 368524770b..4d6bf801db 100644 --- a/packages/php-wasm/universal/src/lib/base-php.ts +++ b/packages/php-wasm/universal/src/lib/base-php.ts @@ -377,7 +377,7 @@ export abstract class BasePHP implements IsomorphicLocalPHP { let errorListener: any; try { // eslint-disable-next-line no-async-promise-executor - exitCode = await new Promise(async (resolve, reject) => { + exitCode = await new Promise((resolve, reject) => { errorListener = (e: ErrorEvent) => { const rethrown = new Error('Rethrown'); rethrown.cause = e.error; @@ -397,7 +397,7 @@ export abstract class BasePHP implements IsomorphicLocalPHP { * * @TODO: Determine whether this is a bug in emscripten or in our code. */ - await await this[__private__dont__use].ccall( + this[__private__dont__use].ccall( 'wasm_sapi_handle_request', NUMBER, [], From df0ef5da2528d22b45f70a9bde922c62d6fc0b53 Mon Sep 17 00:00:00 2001 From: Antony Agrios Date: Fri, 12 May 2023 19:17:08 +0300 Subject: [PATCH 02/10] Fix: return a 404 if the requested path doesnt exit --- .../universal/src/lib/php-request-handler.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/php-wasm/universal/src/lib/php-request-handler.ts b/packages/php-wasm/universal/src/lib/php-request-handler.ts index 16c50378d9..418414c161 100644 --- a/packages/php-wasm/universal/src/lib/php-request-handler.ts +++ b/packages/php-wasm/universal/src/lib/php-request-handler.ts @@ -238,6 +238,14 @@ export class PHPRequestHandler implements RequestHandler { } else { body = request.body; } + const scriptPath = this.#resolvePHPFilePath(requestedUrl.pathname); + if (scriptPath === '') { + return new PHPResponse( + 404, + {}, + new TextEncoder().encode('404 File not found') + ); + } return await this.php.run({ relativeUri: ensurePathPrefix( @@ -284,7 +292,10 @@ export class PHPRequestHandler implements RequestHandler { if (this.php.fileExists(resolvedFsPath)) { return resolvedFsPath; } - return `${this.#DOCROOT}/index.php`; + if (this.php.fileExists(`${this.#DOCROOT}/index.php`)) { + return `${this.#DOCROOT}/index.php`; + } + return ''; } } From 4eeb9216115bb3393bb7219b9a5a79e9981da729 Mon Sep 17 00:00:00 2001 From: Antony Agrios Date: Mon, 15 May 2023 09:32:46 +0300 Subject: [PATCH 03/10] Update: throw an error instead of return empty string in resolvePHPFilePath --- .../universal/src/lib/php-request-handler.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/php-wasm/universal/src/lib/php-request-handler.ts b/packages/php-wasm/universal/src/lib/php-request-handler.ts index 418414c161..ecd91f2fbc 100644 --- a/packages/php-wasm/universal/src/lib/php-request-handler.ts +++ b/packages/php-wasm/universal/src/lib/php-request-handler.ts @@ -238,8 +238,11 @@ export class PHPRequestHandler implements RequestHandler { } else { body = request.body; } - const scriptPath = this.#resolvePHPFilePath(requestedUrl.pathname); - if (scriptPath === '') { + + let scriptPath; + try { + scriptPath = this.#resolvePHPFilePath(requestedUrl.pathname); + } catch (error) { return new PHPResponse( 404, {}, @@ -256,7 +259,7 @@ export class PHPRequestHandler implements RequestHandler { method: request.method || preferredMethod, body, fileInfos, - scriptPath: this.#resolvePHPFilePath(requestedUrl.pathname), + scriptPath, headers, }); } finally { @@ -270,6 +273,7 @@ export class PHPRequestHandler implements RequestHandler { * Fall back to index.php as if there was a url rewriting rule in place. * * @param requestedPath - The requested pathname. + * @throws {Error} If the requested path doesn't exist. * @returns The resolved filesystem path. */ #resolvePHPFilePath(requestedPath: string): string { @@ -295,7 +299,7 @@ export class PHPRequestHandler implements RequestHandler { if (this.php.fileExists(`${this.#DOCROOT}/index.php`)) { return `${this.#DOCROOT}/index.php`; } - return ''; + throw new Error(`File not found: ${resolvedFsPath}`); } } From 03e201229c106fa2a6bda932f99ae02774b0d4c5 Mon Sep 17 00:00:00 2001 From: Antony Agrios Date: Tue, 16 May 2023 13:48:30 +0300 Subject: [PATCH 04/10] Fix: crash due to an unhandled Asyncify error --- .../php-wasm/universal/src/lib/base-php.ts | 27 +++++++------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/packages/php-wasm/universal/src/lib/base-php.ts b/packages/php-wasm/universal/src/lib/base-php.ts index 4d6bf801db..ff5d734fa4 100644 --- a/packages/php-wasm/universal/src/lib/base-php.ts +++ b/packages/php-wasm/universal/src/lib/base-php.ts @@ -388,25 +388,16 @@ export abstract class BasePHP implements IsomorphicLocalPHP { 'error', errorListener ); - - try { - resolve( - /** - * This is awkward, but Asyncify makes wasm_sapi_handle_request return - * Promise>. - * - * @TODO: Determine whether this is a bug in emscripten or in our code. - */ - this[__private__dont__use].ccall( - 'wasm_sapi_handle_request', - NUMBER, - [], - [] - ) - ); - } catch (e) { - reject(e); + const response = this[__private__dont__use].ccall( + 'wasm_sapi_handle_request', + NUMBER, + [], + [] + ); + if (response instanceof Promise) { + return response.then(resolve).catch(reject); } + return resolve(response); }); } catch (e) { /** From 2bd9f551007b2e5b735fe314f44609c494b80c60 Mon Sep 17 00:00:00 2001 From: Antony Agrios Date: Tue, 16 May 2023 14:26:11 +0300 Subject: [PATCH 05/10] Add: test for testing unhandled Asyncify error --- packages/php-wasm/node/src/test/php.spec.ts | 43 +++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/packages/php-wasm/node/src/test/php.spec.ts b/packages/php-wasm/node/src/test/php.spec.ts index 9f53153ba2..500cd00372 100644 --- a/packages/php-wasm/node/src/test/php.spec.ts +++ b/packages/php-wasm/node/src/test/php.spec.ts @@ -554,3 +554,46 @@ bar1 }); }); }); + +// @TODO Prevent crash on PHP versions 5.6, 7.2, 8.2 +describe.each(['7.0','7.1','7.3','7.4','8.0','8.1'])('PHP %s – process crash', (phpVersion) => { + let php: NodePHP; + beforeEach(async () => { + php = await NodePHP.load(phpVersion as any); + php.setPhpIniEntry('allow_url_fopen', '1'); + }); + + it('Does not crash due to an unhandled Asyncify error ', async () => { + let caughtError; + try { + /** + * PHP is intentionally built without network support for __clone() + * because it's an extremely unlikely place for any network activity + * and not supporting it allows us to test the error handling here. + * + * `clone $x` will throw an asynchronous error out when attempting + * to do a network call ("unreachable" WASM instruction executed). + * This test should gracefully catch and handle that error. + * + * A failure to do so will crash the entire process + */ + await php.run({ + code: ` Date: Tue, 16 May 2023 15:00:49 +0300 Subject: [PATCH 06/10] Fix: format issues --- packages/php-wasm/node/src/test/php.spec.ts | 71 +++++++++++---------- 1 file changed, 38 insertions(+), 33 deletions(-) diff --git a/packages/php-wasm/node/src/test/php.spec.ts b/packages/php-wasm/node/src/test/php.spec.ts index 500cd00372..f54a630e0e 100644 --- a/packages/php-wasm/node/src/test/php.spec.ts +++ b/packages/php-wasm/node/src/test/php.spec.ts @@ -556,29 +556,31 @@ bar1 }); // @TODO Prevent crash on PHP versions 5.6, 7.2, 8.2 -describe.each(['7.0','7.1','7.3','7.4','8.0','8.1'])('PHP %s – process crash', (phpVersion) => { - let php: NodePHP; - beforeEach(async () => { - php = await NodePHP.load(phpVersion as any); - php.setPhpIniEntry('allow_url_fopen', '1'); - }); - - it('Does not crash due to an unhandled Asyncify error ', async () => { - let caughtError; - try { - /** - * PHP is intentionally built without network support for __clone() - * because it's an extremely unlikely place for any network activity - * and not supporting it allows us to test the error handling here. - * - * `clone $x` will throw an asynchronous error out when attempting - * to do a network call ("unreachable" WASM instruction executed). - * This test should gracefully catch and handle that error. - * - * A failure to do so will crash the entire process - */ - await php.run({ - code: ` { + let php: NodePHP; + beforeEach(async () => { + php = await NodePHP.load(phpVersion as any); + php.setPhpIniEntry('allow_url_fopen', '1'); + }); + + it('Does not crash due to an unhandled Asyncify error ', async () => { + let caughtError; + try { + /** + * PHP is intentionally built without network support for __clone() + * because it's an extremely unlikely place for any network activity + * and not supporting it allows us to test the error handling here. + * + * `clone $x` will throw an asynchronous error out when attempting + * to do a network call ("unreachable" WASM instruction executed). + * This test should gracefully catch and handle that error. + * + * A failure to do so will crash the entire process + */ + await php.run({ + code: ` Date: Tue, 16 May 2023 15:19:20 +0300 Subject: [PATCH 07/10] Update: pass reject on then instead of using catch --- packages/php-wasm/universal/src/lib/base-php.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/php-wasm/universal/src/lib/base-php.ts b/packages/php-wasm/universal/src/lib/base-php.ts index daa66b8373..bf315f3d40 100644 --- a/packages/php-wasm/universal/src/lib/base-php.ts +++ b/packages/php-wasm/universal/src/lib/base-php.ts @@ -403,7 +403,7 @@ export abstract class BasePHP implements IsomorphicLocalPHP { [] ); if (response instanceof Promise) { - return response.then(resolve).catch(reject); + return response.then(resolve, reject); } return resolve(response); }); From 056fa42f43a4cf71e5ed643f83ed0a200182bad2 Mon Sep 17 00:00:00 2001 From: Antony Agrios Date: Thu, 18 May 2023 11:47:14 +0300 Subject: [PATCH 08/10] Update: Fix and add new tests - Fix testing for current tests for Asyncify error (always passing before) - Add a test to handle no promise cases --- packages/php-wasm/node/src/test/php.spec.ts | 34 +++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/packages/php-wasm/node/src/test/php.spec.ts b/packages/php-wasm/node/src/test/php.spec.ts index f54a630e0e..b0b7a1bc3f 100644 --- a/packages/php-wasm/node/src/test/php.spec.ts +++ b/packages/php-wasm/node/src/test/php.spec.ts @@ -1,5 +1,9 @@ import { getPHPLoaderModule, NodePHP } from '..'; -import { loadPHPRuntime, SupportedPHPVersions } from '@php-wasm/universal'; +import { + loadPHPRuntime, + SupportedPHPVersions, + __private__dont__use, +} from '@php-wasm/universal'; import { existsSync, rmSync, readFileSync } from 'fs'; const testDirPath = '/__test987654321'; @@ -592,7 +596,7 @@ describe.each(['7.0', '7.1', '7.3', '7.4', '8.0', '8.1'])( }); } catch (error) { caughtError = error; - expect(error).toMatch( + expect(error.message).toMatch( /Aborted|Program terminated with exit\(1\)|/ ); } @@ -600,5 +604,31 @@ describe.each(['7.0', '7.1', '7.3', '7.4', '8.0', '8.1'])( expect.fail('php.run should have thrown an error'); } }); + + it('Does not crash due to an unhandled non promise error ', async () => { + let caughtError; + try { + const spy = vi.spyOn(php[__private__dont__use], 'ccall'); + expect(spy.getMockName()).toEqual('ccall'); + spy.mockImplementationOnce(() => { + throw new Error('test'); + }); + + await php.run({ + code: ` Date: Thu, 18 May 2023 14:41:15 +0300 Subject: [PATCH 09/10] Update: Use guards when handling the error --- packages/php-wasm/universal/src/lib/php-request-handler.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/php-wasm/universal/src/lib/php-request-handler.ts b/packages/php-wasm/universal/src/lib/php-request-handler.ts index ecd91f2fbc..55993ae9cb 100644 --- a/packages/php-wasm/universal/src/lib/php-request-handler.ts +++ b/packages/php-wasm/universal/src/lib/php-request-handler.ts @@ -296,10 +296,10 @@ export class PHPRequestHandler implements RequestHandler { if (this.php.fileExists(resolvedFsPath)) { return resolvedFsPath; } - if (this.php.fileExists(`${this.#DOCROOT}/index.php`)) { - return `${this.#DOCROOT}/index.php`; + if (!this.php.fileExists(`${this.#DOCROOT}/index.php`)) { + throw new Error(`File not found: ${resolvedFsPath}`); } - throw new Error(`File not found: ${resolvedFsPath}`); + return `${this.#DOCROOT}/index.php`; } } From 7c4e0d7e9d18a6bd1d27e86fe31e44c650155cdf Mon Sep 17 00:00:00 2001 From: Antony Agrios Date: Thu, 18 May 2023 15:04:23 +0300 Subject: [PATCH 10/10] Tests: thrown only on wasm_sapi_handle_request --- packages/php-wasm/node/src/test/php.spec.ts | 26 ++++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/packages/php-wasm/node/src/test/php.spec.ts b/packages/php-wasm/node/src/test/php.spec.ts index b0b7a1bc3f..902ebd438a 100644 --- a/packages/php-wasm/node/src/test/php.spec.ts +++ b/packages/php-wasm/node/src/test/php.spec.ts @@ -567,6 +567,7 @@ describe.each(['7.0', '7.1', '7.3', '7.4', '8.0', '8.1'])( beforeEach(async () => { php = await NodePHP.load(phpVersion as any); php.setPhpIniEntry('allow_url_fopen', '1'); + vi.restoreAllMocks(); }); it('Does not crash due to an unhandled Asyncify error ', async () => { @@ -594,11 +595,13 @@ describe.each(['7.0', '7.1', '7.3', '7.4', '8.0', '8.1'])( clone $x; `, }); - } catch (error) { + } catch (error: unknown) { caughtError = error; - expect(error.message).toMatch( - /Aborted|Program terminated with exit\(1\)|/ - ); + if (error instanceof Error) { + expect(error.message).toMatch( + /Aborted|Program terminated with exit\(1\)|/ + ); + } } if (!caughtError) { expect.fail('php.run should have thrown an error'); @@ -610,21 +613,26 @@ describe.each(['7.0', '7.1', '7.3', '7.4', '8.0', '8.1'])( try { const spy = vi.spyOn(php[__private__dont__use], 'ccall'); expect(spy.getMockName()).toEqual('ccall'); - spy.mockImplementationOnce(() => { - throw new Error('test'); + spy.mockImplementation((c_func) => { + if (c_func === 'wasm_sapi_handle_request') { + throw new Error('test'); + } }); await php.run({ code: `