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 <[email protected]> * Update frankenphp_test.go Co-authored-by: Kévin Dunglas <[email protected]> --------- Co-authored-by: Alliballibaba <[email protected]> Co-authored-by: Kévin Dunglas <[email protected]>
dunglas · Jan 25, 2025 · 05aafc7 · 05aafc7
1 parent eee1de1
commit 05aafc7
Show file tree

Hide file tree

Showing 5 changed files with 68 additions and 20 deletions.
diff --git a/frankenphp.c b/frankenphp.c
@@ -135,6 +135,23 @@ static void frankenphp_worker_request_shutdown() {
   zend_end_try();
 
   zend_set_memory_limit(PG(memory_limit));
+
+  /*
+   * free any php_stream resources that are not php source files
+   * all resources are stored in EG(regular_list), see zend_list.c
+   */
+  zend_resource *val;
+  ZEND_HASH_FOREACH_PTR(&EG(regular_list), val) {
+    /* verify the resource is a stream */
+    if (val->type == php_file_le_stream()) {
+      php_stream *stream = (php_stream *)val->ptr;
+      if (stream != NULL && stream->ops != &php_stream_stdio_ops &&
+          !stream->is_persistent && GC_REFCOUNT(val) == 1) {
+        zend_list_delete(val);
+      }
+    }
+  }
+  ZEND_HASH_FOREACH_END();
 }
 
 PHPAPI void get_full_env(zval *track_vars_array) {
@@ -746,7 +763,7 @@ void frankenphp_register_variables_from_request_info(
     zend_string *path_translated, zend_string *query_string,
     zend_string *auth_user, zend_string *request_method) {
   frankenphp_register_variable_from_request_info(
-      content_type, (char *)SG(request_info).content_type, false,
+      content_type, (char *)SG(request_info).content_type, true,
       track_vars_array);
   frankenphp_register_variable_from_request_info(
       path_translated, (char *)SG(request_info).path_translated, false,
@@ -904,6 +921,9 @@ static void *php_thread(void *arg) {
 
   go_frankenphp_on_thread_shutdown(thread_index);
 
+  free(local_ctx);
+  local_ctx = NULL;
+
   return NULL;
 }
 

diff --git a/frankenphp_test.go b/frankenphp_test.go
@@ -37,14 +37,14 @@ import (
 )
 
 type testOptions struct {
-	workerScript        string
-	watch               []string
-	nbWorkers           int
-	env                 map[string]string
-	nbParallelRequests  int
-	realServer          bool
-	logger              *zap.Logger
-	initOpts            []frankenphp.Option
+	workerScript       string
+	watch              []string
+	nbWorkers          int
+	env                map[string]string
+	nbParallelRequests int
+	realServer         bool
+	logger             *zap.Logger
+	initOpts           []frankenphp.Option
 }
 
 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) {
 	}
 }
 
+// Worker mode will clean up unreferenced streams between requests
+// Make sure referenced streams are not cleaned up
+func TestFileStreamInWorkerMode(t *testing.T) {
+	runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, _ int) {
+		resp1 := fetchBody("GET", "http://example.com/file-stream.php", handler)
+		assert.Equal(t, resp1, "word1")
+
+		resp2 := fetchBody("GET", "http://example.com/file-stream.php", handler)
+		assert.Equal(t, resp2, "word2")
+
+		resp3 := fetchBody("GET", "http://example.com/file-stream.php", handler)
+		assert.Equal(t, resp3, "word3")
+	}, &testOptions{workerScript: "file-stream.php", nbParallelRequests: 1, nbWorkers: 1})
+}
+
 // To run this fuzzing test use: go test -fuzz FuzzRequest
 // TODO: Cover more potential cases
 func FuzzRequest(f *testing.F) {
@@ -978,3 +993,13 @@ func FuzzRequest(f *testing.F) {
 		}, &testOptions{workerScript: "request-headers.php"})
 	})
 }
+
+func fetchBody(method string, url string, handler func(http.ResponseWriter, *http.Request)) string {
+	req := httptest.NewRequest(method, url, nil)
+	w := httptest.NewRecorder()
+	handler(w, req)
+	resp := w.Result()
+	body, _ := io.ReadAll(resp.Body)
+
+	return string(body)
+}
diff --git a/testdata/file-stream.php b/testdata/file-stream.php
@@ -0,0 +1,13 @@
+<?php
+
+$fileStream = fopen(__DIR__ . '/file-stream.txt', 'r');
+$input = fopen('php://input', 'r');
+
+while (frankenphp_handle_request(function () use ($fileStream, $input) {
+    echo fread($fileStream, 5);
+
+    // this line will lead to a zend_mm_heap corrupted error if the input stream was destroyed
+    stream_is_local($input);
+})) ;
+
+fclose($fileStream);
diff --git a/testdata/file-stream.txt b/testdata/file-stream.txt
@@ -0,0 +1 @@
+word1word2word3
diff --git a/watcher_test.go b/watcher_test.go
@@ -3,7 +3,6 @@
 package frankenphp_test
 
 import (
-	"io"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -41,16 +40,6 @@ func TestWorkersShouldNotReloadOnExcludingPattern(t *testing.T) {
 	}, &testOptions{nbParallelRequests: 1, nbWorkers: 1, workerScript: "worker-with-watcher.php", watch: watch})
 }
 
-func fetchBody(method string, url string, handler func(http.ResponseWriter, *http.Request)) string {
-	req := httptest.NewRequest(method, url, nil)
-	w := httptest.NewRecorder()
-	handler(w, req)
-	resp := w.Result()
-	body, _ := io.ReadAll(resp.Body)
-
-	return string(body)
-}
-
 func pollForWorkerReset(t *testing.T, handler func(http.ResponseWriter, *http.Request), limit int) bool {
 	// first we make an initial request to start the request counter
 	body := fetchBody("GET", "http://example.com/worker-with-watcher.php", handler)