From f31b333ae9082684ba201323de8766ab5f8589a0 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Wed, 14 Aug 2024 20:47:02 +0100 Subject: [PATCH 1/4] Fix GH-15395: avoid copying empty auth password for PHP_AUTH_PW. But we still consider the authentication handling Basic part successful. --- main/main.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/main/main.c b/main/main.c index 0adecd10ffcc..a3889bb3c5b4 100644 --- a/main/main.c +++ b/main/main.c @@ -2672,7 +2672,12 @@ PHPAPI int php_handle_auth_data(const char *auth) if (pass) { *pass++ = '\0'; SG(request_info).auth_user = estrndup(ZSTR_VAL(user), ZSTR_LEN(user)); - SG(request_info).auth_password = estrdup(pass); + + if (strlen(pass) > 0) { + SG(request_info).auth_password = estrdup(pass); + } else { + SG(request_info).auth_password = NULL; + } ret = 0; } zend_string_free(user); From 71f8824e739bb3faf43d638b0b751d01680393ab Mon Sep 17 00:00:00 2001 From: David Carlier Date: Wed, 14 Aug 2024 21:24:02 +0100 Subject: [PATCH 2/4] avoid dangling data during shutdown --- main/SAPI.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/main/SAPI.c b/main/SAPI.c index 09b028e55166..a01b4045cfaa 100644 --- a/main/SAPI.c +++ b/main/SAPI.c @@ -507,18 +507,23 @@ SAPI_API void sapi_deactivate_module(void) } if (SG(request_info).auth_user) { efree(SG(request_info).auth_user); + SG(request_info).auth_user = NULL; } if (SG(request_info).auth_password) { efree(SG(request_info).auth_password); + SG(request_info).auth_password = NULL; } if (SG(request_info).auth_digest) { efree(SG(request_info).auth_digest); + SG(request_info).auth_digest = NULL; } if (SG(request_info).content_type_dup) { efree(SG(request_info).content_type_dup); + SG(request_info).content_type_dup = NULL; } if (SG(request_info).current_user) { efree(SG(request_info).current_user); + SG(request_info).current_user = NULL; } if (sapi_module.deactivate) { sapi_module.deactivate(); From 36b8a2cd807d7fd7d0c2cc437ac612ff1e896dac Mon Sep 17 00:00:00 2001 From: David Carlier Date: Wed, 14 Aug 2024 22:55:08 +0100 Subject: [PATCH 3/4] update the test framework for the http authentication part. adding test. --- sapi/fpm/tests/gh15395.phpt | 63 +++++++++++++++++++++++++++++++++++++ sapi/fpm/tests/tester.inc | 9 ++++-- 2 files changed, 69 insertions(+), 3 deletions(-) create mode 100644 sapi/fpm/tests/gh15395.phpt diff --git a/sapi/fpm/tests/gh15395.phpt b/sapi/fpm/tests/gh15395.phpt new file mode 100644 index 000000000000..822597be7c17 --- /dev/null +++ b/sapi/fpm/tests/gh15395.phpt @@ -0,0 +1,63 @@ +--TEST-- +GH-15335 data logging during shutdown +--SKIPIF-- + +--FILE-- +createSourceFileAndScriptName(); +$tester->start(); +$tester->expectLogStartNotices(); +$tester + ->request( + uri: $scriptName, + address: '{{ADDR:UDS}}', + scriptFilename: "/", + scriptName: "/", + httpAuthorization: "Basic QWxhZGRpbjpvcGVuIHNlc2FtZQ==", + ); + +$tester + ->request( + uri: $scriptName, + address: '{{ADDR:UDS}}', + scriptFilename: "/", + scriptName: "/", + ); +$tester->terminate(); +$tester->expectLogTerminatingNotices(); +$tester->close(); + +?> +Done +--EXPECT-- +Done +--CLEAN-- + + diff --git a/sapi/fpm/tests/tester.inc b/sapi/fpm/tests/tester.inc index ed0988f883ac..0036d16ee672 100644 --- a/sapi/fpm/tests/tester.inc +++ b/sapi/fpm/tests/tester.inc @@ -627,7 +627,8 @@ class Tester string $uri = null, string $scriptFilename = null, string $scriptName = null, - ?string $stdin = null + ?string $stdin = null, + ?string $httpAuthorization = null ): array { if (is_null($scriptFilename)) { $scriptFilename = $this->makeSourceFile(); @@ -657,7 +658,8 @@ class Tester 'SERVER_PROTOCOL' => 'HTTP/1.1', 'DOCUMENT_ROOT' => __DIR__, 'CONTENT_TYPE' => '', - 'CONTENT_LENGTH' => strlen($stdin ?? "") // Default to 0 + 'CONTENT_LENGTH' => strlen($stdin ?? ""), // Default to 0 + 'HTTP_AUTHORIZATION' => $httpAuthorization ? $httpAuthorization : "" ], $headers ); @@ -762,6 +764,7 @@ class Tester string|array $stdin = null, bool $expectError = false, int $readLimit = -1, + string $httpAuthorization = null, ): Response { if ($this->hasError()) { return $this->createResponse(expectInvalid: true); @@ -771,7 +774,7 @@ class Tester $stdin = $this->parseStdin($stdin, $headers); } - $params = $this->getRequestParams($query, $headers, $uri, $scriptFilename, $scriptName, $stdin); + $params = $this->getRequestParams($query, $headers, $uri, $scriptFilename, $scriptName, $stdin, $httpAuthorization); $this->trace('Request params', $params); try { From b88425cc3a8974f6fac1f5c4a9b6bf9f3a0212fc Mon Sep 17 00:00:00 2001 From: David Carlier Date: Wed, 14 Aug 2024 23:24:12 +0100 Subject: [PATCH 4/4] fix test --- main/SAPI.c | 2 -- sapi/fpm/tests/gh15395.phpt | 31 ++++++++++++++++++++----------- sapi/fpm/tests/tester.inc | 9 +++------ 3 files changed, 23 insertions(+), 19 deletions(-) diff --git a/main/SAPI.c b/main/SAPI.c index a01b4045cfaa..d0226ded65be 100644 --- a/main/SAPI.c +++ b/main/SAPI.c @@ -519,11 +519,9 @@ SAPI_API void sapi_deactivate_module(void) } if (SG(request_info).content_type_dup) { efree(SG(request_info).content_type_dup); - SG(request_info).content_type_dup = NULL; } if (SG(request_info).current_user) { efree(SG(request_info).current_user); - SG(request_info).current_user = NULL; } if (sapi_module.deactivate) { sapi_module.deactivate(); diff --git a/sapi/fpm/tests/gh15395.phpt b/sapi/fpm/tests/gh15395.phpt index 822597be7c17..82abca85e09c 100644 --- a/sapi/fpm/tests/gh15395.phpt +++ b/sapi/fpm/tests/gh15395.phpt @@ -33,20 +33,29 @@ $tester->start(); $tester->expectLogStartNotices(); $tester ->request( - uri: $scriptName, - address: '{{ADDR:UDS}}', - scriptFilename: "/", - scriptName: "/", - httpAuthorization: "Basic QWxhZGRpbjpvcGVuIHNlc2FtZQ==", - ); + uri: $scriptName, + address: '{{ADDR:UDS}}', + scriptFilename: "/", + scriptName: "/", + headers: ["HTTP_AUTHORIZATION" => "Basic QWxhZGRpbjpvcGVuIHNlc2FtZQ==", "REQUEST_METHOD" => "GET"], + ) + ->expectStatus('404 Not Found'); +$tester->terminate(); +$tester->expectLogTerminatingNotices(); +$tester->close(); +$tester = new FPM\Tester($cfg, $code); +[$sourceFilePath, $scriptName] = $tester->createSourceFileAndScriptName(); +$tester->start(); +$tester->expectLogStartNotices(); $tester ->request( - uri: $scriptName, - address: '{{ADDR:UDS}}', - scriptFilename: "/", - scriptName: "/", - ); + uri: $scriptName, + address: '{{ADDR:UDS}}', + scriptFilename: $sourceFilePath, + scriptName: $scriptName, + ) + ->expectBody([$scriptName, $sourceFilePath, $scriptName]); $tester->terminate(); $tester->expectLogTerminatingNotices(); $tester->close(); diff --git a/sapi/fpm/tests/tester.inc b/sapi/fpm/tests/tester.inc index 0036d16ee672..ed0988f883ac 100644 --- a/sapi/fpm/tests/tester.inc +++ b/sapi/fpm/tests/tester.inc @@ -627,8 +627,7 @@ class Tester string $uri = null, string $scriptFilename = null, string $scriptName = null, - ?string $stdin = null, - ?string $httpAuthorization = null + ?string $stdin = null ): array { if (is_null($scriptFilename)) { $scriptFilename = $this->makeSourceFile(); @@ -658,8 +657,7 @@ class Tester 'SERVER_PROTOCOL' => 'HTTP/1.1', 'DOCUMENT_ROOT' => __DIR__, 'CONTENT_TYPE' => '', - 'CONTENT_LENGTH' => strlen($stdin ?? ""), // Default to 0 - 'HTTP_AUTHORIZATION' => $httpAuthorization ? $httpAuthorization : "" + 'CONTENT_LENGTH' => strlen($stdin ?? "") // Default to 0 ], $headers ); @@ -764,7 +762,6 @@ class Tester string|array $stdin = null, bool $expectError = false, int $readLimit = -1, - string $httpAuthorization = null, ): Response { if ($this->hasError()) { return $this->createResponse(expectInvalid: true); @@ -774,7 +771,7 @@ class Tester $stdin = $this->parseStdin($stdin, $headers); } - $params = $this->getRequestParams($query, $headers, $uri, $scriptFilename, $scriptName, $stdin, $httpAuthorization); + $params = $this->getRequestParams($query, $headers, $uri, $scriptFilename, $scriptName, $stdin); $this->trace('Request params', $params); try {