Skip to content

Commit 05aafc7

Browse files
withinboredomAlliBalliBabadunglas
authored
fix memory leaks (#1350)
* fix a memory leak on thread shutdown * clean up unused resources at end of request * try the obvious * Test * clang-format * Also ignores persistent streams. * Adds stream test. * Moves clean up function to frankenphp_worker_request_shutdown. * Fixes test on -nowatcher * Fixes test on -nowatcher * Update testdata/file-stream.txt Co-authored-by: Kévin Dunglas <kevin@dunglas.fr> * Update frankenphp_test.go Co-authored-by: Kévin Dunglas <kevin@dunglas.fr> --------- Co-authored-by: Alliballibaba <alliballibaba@gmail.com> Co-authored-by: Kévin Dunglas <kevin@dunglas.fr>
1 parent eee1de1 commit 05aafc7

5 files changed

Lines changed: 68 additions & 20 deletions

File tree

frankenphp.c

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,23 @@ static void frankenphp_worker_request_shutdown() {
135135
zend_end_try();
136136

137137
zend_set_memory_limit(PG(memory_limit));
138+
139+
/*
140+
* free any php_stream resources that are not php source files
141+
* all resources are stored in EG(regular_list), see zend_list.c
142+
*/
143+
zend_resource *val;
144+
ZEND_HASH_FOREACH_PTR(&EG(regular_list), val) {
145+
/* verify the resource is a stream */
146+
if (val->type == php_file_le_stream()) {
147+
php_stream *stream = (php_stream *)val->ptr;
148+
if (stream != NULL && stream->ops != &php_stream_stdio_ops &&
149+
!stream->is_persistent && GC_REFCOUNT(val) == 1) {
150+
zend_list_delete(val);
151+
}
152+
}
153+
}
154+
ZEND_HASH_FOREACH_END();
138155
}
139156

140157
PHPAPI void get_full_env(zval *track_vars_array) {
@@ -746,7 +763,7 @@ void frankenphp_register_variables_from_request_info(
746763
zend_string *path_translated, zend_string *query_string,
747764
zend_string *auth_user, zend_string *request_method) {
748765
frankenphp_register_variable_from_request_info(
749-
content_type, (char *)SG(request_info).content_type, false,
766+
content_type, (char *)SG(request_info).content_type, true,
750767
track_vars_array);
751768
frankenphp_register_variable_from_request_info(
752769
path_translated, (char *)SG(request_info).path_translated, false,
@@ -904,6 +921,9 @@ static void *php_thread(void *arg) {
904921

905922
go_frankenphp_on_thread_shutdown(thread_index);
906923

924+
free(local_ctx);
925+
local_ctx = NULL;
926+
907927
return NULL;
908928
}
909929

frankenphp_test.go

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,14 +37,14 @@ import (
3737
)
3838

3939
type testOptions struct {
40-
workerScript string
41-
watch []string
42-
nbWorkers int
43-
env map[string]string
44-
nbParallelRequests int
45-
realServer bool
46-
logger *zap.Logger
47-
initOpts []frankenphp.Option
40+
workerScript string
41+
watch []string
42+
nbWorkers int
43+
env map[string]string
44+
nbParallelRequests int
45+
realServer bool
46+
logger *zap.Logger
47+
initOpts []frankenphp.Option
4848
}
4949

5050
func runTest(t *testing.T, test func(func(http.ResponseWriter, *http.Request), *httptest.Server, int), opts *testOptions) {
@@ -938,6 +938,21 @@ func testRejectInvalidHeaders(t *testing.T, opts *testOptions) {
938938
}
939939
}
940940

941+
// Worker mode will clean up unreferenced streams between requests
942+
// Make sure referenced streams are not cleaned up
943+
func TestFileStreamInWorkerMode(t *testing.T) {
944+
runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, _ int) {
945+
resp1 := fetchBody("GET", "http://example.com/file-stream.php", handler)
946+
assert.Equal(t, resp1, "word1")
947+
948+
resp2 := fetchBody("GET", "http://example.com/file-stream.php", handler)
949+
assert.Equal(t, resp2, "word2")
950+
951+
resp3 := fetchBody("GET", "http://example.com/file-stream.php", handler)
952+
assert.Equal(t, resp3, "word3")
953+
}, &testOptions{workerScript: "file-stream.php", nbParallelRequests: 1, nbWorkers: 1})
954+
}
955+
941956
// To run this fuzzing test use: go test -fuzz FuzzRequest
942957
// TODO: Cover more potential cases
943958
func FuzzRequest(f *testing.F) {
@@ -978,3 +993,13 @@ func FuzzRequest(f *testing.F) {
978993
}, &testOptions{workerScript: "request-headers.php"})
979994
})
980995
}
996+
997+
func fetchBody(method string, url string, handler func(http.ResponseWriter, *http.Request)) string {
998+
req := httptest.NewRequest(method, url, nil)
999+
w := httptest.NewRecorder()
1000+
handler(w, req)
1001+
resp := w.Result()
1002+
body, _ := io.ReadAll(resp.Body)
1003+
1004+
return string(body)
1005+
}

testdata/file-stream.php

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<?php
2+
3+
$fileStream = fopen(__DIR__ . '/file-stream.txt', 'r');
4+
$input = fopen('php://input', 'r');
5+
6+
while (frankenphp_handle_request(function () use ($fileStream, $input) {
7+
echo fread($fileStream, 5);
8+
9+
// this line will lead to a zend_mm_heap corrupted error if the input stream was destroyed
10+
stream_is_local($input);
11+
})) ;
12+
13+
fclose($fileStream);

testdata/file-stream.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
word1word2word3

watcher_test.go

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
package frankenphp_test
44

55
import (
6-
"io"
76
"net/http"
87
"net/http/httptest"
98
"os"
@@ -41,16 +40,6 @@ func TestWorkersShouldNotReloadOnExcludingPattern(t *testing.T) {
4140
}, &testOptions{nbParallelRequests: 1, nbWorkers: 1, workerScript: "worker-with-watcher.php", watch: watch})
4241
}
4342

44-
func fetchBody(method string, url string, handler func(http.ResponseWriter, *http.Request)) string {
45-
req := httptest.NewRequest(method, url, nil)
46-
w := httptest.NewRecorder()
47-
handler(w, req)
48-
resp := w.Result()
49-
body, _ := io.ReadAll(resp.Body)
50-
51-
return string(body)
52-
}
53-
5443
func pollForWorkerReset(t *testing.T, handler func(http.ResponseWriter, *http.Request), limit int) bool {
5544
// first we make an initial request to start the request counter
5645
body := fetchBody("GET", "http://example.com/worker-with-watcher.php", handler)

0 commit comments

Comments
 (0)