perf: optimize $_SERVER import (#1106)

Co-authored-by: Kévin Dunglas <kevin@dunglas.fr>
Co-authored-by: a.stecher <a.stecher@sportradar.com>
Co-authored-by: Alliballibaba <alliballibaba@gmail.com>

.github/workflows/tests.yaml

@@ -64,6 +64,10 @@ jobs:
         name: Run Caddy module tests
         working-directory: caddy/
         run: go test -tags nobadger,nomysql,nopgx -race -v ./...
+      -
+        name: Run Fuzzing Tests
+        working-directory: caddy/
+        run: go test -fuzz FuzzRequest -fuzztime 20s
       -
         name: Build the server
         working-directory: caddy/frankenphp/

caddy/caddy_test.go

@@ -4,6 +4,8 @@ import (
 	"bytes"
 	"fmt"
 	"net/http"
+	"os"
+	"path/filepath"
 	"strings"
 	"sync"
 	"testing"
@@ -575,3 +577,39 @@ func TestAutoWorkerConfig(t *testing.T) {
 			"frankenphp_testdata_index_php_ready_workers",
 		))
 }
+
+func TestAllDefinedServerVars(t *testing.T) {
+	documentRoot, _ := filepath.Abs("../testdata/")
+	expectedBodyFile, _ := os.ReadFile("../testdata/server-all-vars-ordered.txt")
+	expectedBody := string(expectedBodyFile)
+	expectedBody = strings.ReplaceAll(expectedBody, "{documentRoot}", documentRoot)
+	expectedBody = strings.ReplaceAll(expectedBody, "\r\n", "\n")
+	expectedBody = strings.ReplaceAll(expectedBody, "{testPort}", testPort)
+	tester := caddytest.NewTester(t)
+	tester.InitServer(`
+		{
+			skip_install_trust
+			admin localhost:2999
+			http_port `+testPort+`
+			frankenphp
+		}
+		localhost:`+testPort+` {
+			route {
+			    root ../testdata
+				php
+			}
+		}
+		`, "caddyfile")
+	tester.AssertPostResponseBody(
+		"http://user@localhost:"+testPort+"/server-all-vars-ordered.php/path?specialChars=%3E\\x00%00</>",
+		[]string{
+			"Content-Type: application/x-www-form-urlencoded",
+			"Content-Length: 14", // maliciously set to 14
+			"Special-Chars: <%00>",
+			"Host: Malicous Host",
+		},
+		bytes.NewBufferString("foo=bar"),
+		http.StatusOK,
+		expectedBody,
+	)
+}

cgi.go

@@ -1,5 +1,6 @@
 package frankenphp
 
+// #include <php_variables.h>
 // #include "frankenphp.h"
 import "C"
 import (
@@ -7,33 +8,7 @@ import (
 	"net"
 	"net/http"
 	"path/filepath"
-	"runtime"
 	"strings"
-	"unsafe"
-)
-
-type serverKey int
-
-const (
-	contentLength serverKey = iota
-	documentRoot
-	documentUri
-	gatewayInterface
-	httpHost
-	https
-	pathInfo
-	phpSelf
-	remoteAddr
-	remoteHost
-	remotePort
-	requestScheme
-	scriptFilename
-	scriptName
-	serverName
-	serverPort
-	serverProtocol
-	serverSoftware
-	sslProtocol
 )
 
 var knownServerKeys = map[string]struct{}{
@@ -56,29 +31,22 @@ var knownServerKeys = map[string]struct{}{
 	"SERVER_PROTOCOL\x00":   {},
 	"SERVER_SOFTWARE\x00":   {},
 	"SSL_PROTOCOL\x00":      {},
-}
-
-func setKnownServerVariable(p *runtime.Pinner, cArr *[27]C.go_string, serverKey serverKey, val string) {
-	if val == "" {
-		return
-	}
-
-	valData := unsafe.StringData(val)
-	p.Pin(valData)
-	cArr[serverKey].len = C.size_t(len(val))
-	cArr[serverKey].data = (*C.char)(unsafe.Pointer(valData))
+	"AUTH_TYPE\x00":         {},
+	"REMOTE_IDENT\x00":      {},
+	"CONTENT_TYPE\x00":      {},
+	"PATH_TRANSLATED\x00":   {},
+	"QUERY_STRING\x00":      {},
+	"REMOTE_USER\x00":       {},
+	"REQUEST_METHOD\x00":    {},
+	"REQUEST_URI\x00":       {},
 }
 
 // computeKnownVariables returns a set of CGI environment variables for the request.
 //
 // TODO: handle this case https://github.com/caddyserver/caddy/issues/3718
 // Inspired by https://github.com/caddyserver/caddy/blob/master/modules/caddyhttp/reverseproxy/fastcgi/fastcgi.go
-func computeKnownVariables(request *http.Request, p *runtime.Pinner) (cArr [27]C.go_string) {
-	fc, fcOK := FromContext(request.Context())
-	if !fcOK {
-		panic("not a FrankenPHP request")
-	}
-
+func addKnownVariablesToServer(thread *phpThread, request *http.Request, fc *FrankenPHPContext, track_vars_array *C.zval) {
+	keys := getKnownVariableKeys(thread)
 	// Separate remote IP and port; more lenient than net.SplitHostPort
 	var ip, port string
 	if idx := strings.LastIndex(request.RemoteAddr, ":"); idx > -1 {
@@ -94,53 +62,56 @@ func computeKnownVariables(request *http.Request, p *runtime.Pinner) (cArr [27]C
 
 	ra, raOK := fc.env["REMOTE_ADDR\x00"]
 	if raOK {
-		setKnownServerVariable(p, &cArr, remoteAddr, ra)
+		registerTrustedVar(keys["REMOTE_ADDR\x00"], ra, track_vars_array, thread)
 	} else {
-		setKnownServerVariable(p, &cArr, remoteAddr, ip)
+		registerTrustedVar(keys["REMOTE_ADDR\x00"], ip, track_vars_array, thread)
 	}
 
 	if rh, ok := fc.env["REMOTE_HOST\x00"]; ok {
-		setKnownServerVariable(p, &cArr, remoteHost, rh) // For speed, remote host lookups disabled
+		registerTrustedVar(keys["REMOTE_HOST\x00"], rh, track_vars_array, thread) // For speed, remote host lookups disabled
 	} else {
 		if raOK {
-			setKnownServerVariable(p, &cArr, remoteHost, ip)
+			registerTrustedVar(keys["REMOTE_HOST\x00"], ra, track_vars_array, thread)
 		} else {
-			cArr[remoteHost] = cArr[remoteAddr]
+			registerTrustedVar(keys["REMOTE_HOST\x00"], ip, track_vars_array, thread)
 		}
 	}
 
-	setKnownServerVariable(p, &cArr, remotePort, port)
-	setKnownServerVariable(p, &cArr, documentRoot, fc.documentRoot)
-	setKnownServerVariable(p, &cArr, pathInfo, fc.pathInfo)
-	setKnownServerVariable(p, &cArr, phpSelf, request.URL.Path)
-	setKnownServerVariable(p, &cArr, documentUri, fc.docURI)
-	setKnownServerVariable(p, &cArr, scriptFilename, fc.scriptFilename)
-	setKnownServerVariable(p, &cArr, scriptName, fc.scriptName)
+	registerTrustedVar(keys["REMOTE_PORT\x00"], port, track_vars_array, thread)
+	registerTrustedVar(keys["DOCUMENT_ROOT\x00"], fc.documentRoot, track_vars_array, thread)
+	registerTrustedVar(keys["PATH_INFO\x00"], fc.pathInfo, track_vars_array, thread)
+	registerTrustedVar(keys["PHP_SELF\x00"], request.URL.Path, track_vars_array, thread)
+	registerTrustedVar(keys["DOCUMENT_URI\x00"], fc.docURI, track_vars_array, thread)
+	registerTrustedVar(keys["SCRIPT_FILENAME\x00"], fc.scriptFilename, track_vars_array, thread)
+	registerTrustedVar(keys["SCRIPT_NAME\x00"], fc.scriptName, track_vars_array, thread)
 
 	var rs string
 	if request.TLS == nil {
 		rs = "http"
+		registerTrustedVar(keys["HTTPS\x00"], "", track_vars_array, thread)
+		registerTrustedVar(keys["SSL_PROTOCOL\x00"], "", track_vars_array, thread)
 	} else {
 		rs = "https"
-
 		if h, ok := fc.env["HTTPS\x00"]; ok {
-			setKnownServerVariable(p, &cArr, https, h)
+			registerTrustedVar(keys["HTTPS\x00"], h, track_vars_array, thread)
 		} else {
-			setKnownServerVariable(p, &cArr, https, "on")
+			registerTrustedVar(keys["HTTPS\x00"], "on", track_vars_array, thread)
 		}
 
 		// and pass the protocol details in a manner compatible with apache's mod_ssl
 		// (which is why these have a SSL_ prefix and not TLS_).
 		if pr, ok := fc.env["SSL_PROTOCOL\x00"]; ok {
-			setKnownServerVariable(p, &cArr, sslProtocol, pr)
+			registerTrustedVar(keys["SSL_PROTOCOL\x00"], pr, track_vars_array, thread)
 		} else {
 			if v, ok := tlsProtocolStrings[request.TLS.Version]; ok {
-				setKnownServerVariable(p, &cArr, sslProtocol, v)
+				registerTrustedVar(keys["SSL_PROTOCOL\x00"], v, track_vars_array, thread)
+			} else {
+				registerTrustedVar(keys["SSL_PROTOCOL\x00"], "", track_vars_array, thread)
 			}
 		}
 	}
 
-	setKnownServerVariable(p, &cArr, requestScheme, rs)
+	registerTrustedVar(keys["REQUEST_SCHEME\x00"], rs, track_vars_array, thread)
 	reqHost, reqPort, _ := net.SplitHostPort(request.Host)
 
 	if reqHost == "" {
@@ -161,23 +132,104 @@ func computeKnownVariables(request *http.Request, p *runtime.Pinner) (cArr [27]C
 		}
 	}
 
-	setKnownServerVariable(p, &cArr, serverName, reqHost)
+	registerTrustedVar(keys["SERVER_NAME\x00"], reqHost, track_vars_array, thread)
 	if reqPort != "" {
-		setKnownServerVariable(p, &cArr, serverPort, reqPort)
+		registerTrustedVar(keys["SERVER_PORT\x00"], reqPort, track_vars_array, thread)
+	} else {
+		registerTrustedVar(keys["SERVER_PORT\x00"], "", track_vars_array, thread)
 	}
 
 	// Variables defined in CGI 1.1 spec
 	// Some variables are unused but cleared explicitly to prevent
 	// the parent environment from interfering.
 
-	// These values can not be override
-	setKnownServerVariable(p, &cArr, contentLength, request.Header.Get("Content-Length"))
-	setKnownServerVariable(p, &cArr, gatewayInterface, "CGI/1.1")
-	setKnownServerVariable(p, &cArr, serverProtocol, request.Proto)
-	setKnownServerVariable(p, &cArr, serverSoftware, "FrankenPHP")
-	setKnownServerVariable(p, &cArr, httpHost, request.Host) // added here, since not always part of headers
+	// These values can not be overridden
+	registerTrustedVar(keys["CONTENT_LENGTH\x00"], request.Header.Get("Content-Length"), track_vars_array, thread)
+	registerTrustedVar(keys["GATEWAY_INTERFACE\x00"], "CGI/1.1", track_vars_array, thread)
+	registerTrustedVar(keys["SERVER_PROTOCOL\x00"], request.Proto, track_vars_array, thread)
+	registerTrustedVar(keys["SERVER_SOFTWARE\x00"], "FrankenPHP", track_vars_array, thread)
+	registerTrustedVar(keys["HTTP_HOST\x00"], request.Host, track_vars_array, thread) // added here, since not always part of headers
+
+	// These values are always empty but must be defined:
+	registerTrustedVar(keys["AUTH_TYPE\x00"], "", track_vars_array, thread)
+	registerTrustedVar(keys["REMOTE_IDENT\x00"], "", track_vars_array, thread)
+
+	// These values are already present in the SG(request_info), so we'll register them from there
+	C.frankenphp_register_variables_from_request_info(
+		track_vars_array,
+		keys["CONTENT_TYPE\x00"],
+		keys["PATH_TRANSLATED\x00"],
+		keys["QUERY_STRING\x00"],
+		keys["REMOTE_USER\x00"],
+		keys["REQUEST_METHOD\x00"],
+		keys["REQUEST_URI\x00"],
+	)
+}
+
+func registerTrustedVar(key *C.zend_string, value string, track_vars_array *C.zval, thread *phpThread) {
+	C.frankenphp_register_trusted_var(key, thread.pinString(value), C.int(len(value)), track_vars_array)
+}
+
+func addHeadersToServer(thread *phpThread, request *http.Request, fc *FrankenPHPContext, track_vars_array *C.zval) {
+	for field, val := range request.Header {
+		k, ok := headerKeyCache.Get(field)
+		if !ok {
+			k = "HTTP_" + headerNameReplacer.Replace(strings.ToUpper(field)) + "\x00"
+			headerKeyCache.SetIfAbsent(field, k)
+		}
+
+		if _, ok := fc.env[k]; ok {
+			continue
+		}
+
+		v := strings.Join(val, ", ")
+		C.frankenphp_register_variable_safe(thread.pinString(k), thread.pinString(v), C.size_t(len(v)), track_vars_array)
+	}
+}
+
+func addPreparedEnvToServer(thread *phpThread, fc *FrankenPHPContext, track_vars_array *C.zval) {
+	for k, v := range fc.env {
+		C.frankenphp_register_variable_safe(thread.pinString(k), thread.pinString(v), C.size_t(len(v)), track_vars_array)
+	}
+	fc.env = nil
+}
+
+func getKnownVariableKeys(thread *phpThread) map[string]*C.zend_string {
+	if thread.knownVariableKeys != nil {
+		return thread.knownVariableKeys
+	}
+	threadServerKeys := make(map[string]*C.zend_string)
+	for k, _ := range knownServerKeys {
+		keyWithoutNull := strings.Replace(k, "\x00", "", -1)
+		threadServerKeys[k] = C.frankenphp_init_persistent_string(thread.pinString(keyWithoutNull), C.size_t(len(keyWithoutNull)))
+	}
+	thread.knownVariableKeys = threadServerKeys
+	return threadServerKeys
+}
 
-	return
+//export go_register_variables
+func go_register_variables(threadIndex C.uintptr_t, trackVarsArray *C.zval) {
+	thread := phpThreads[threadIndex]
+	r := thread.getActiveRequest()
+	fc := r.Context().Value(contextKey).(*FrankenPHPContext)
+
+	addKnownVariablesToServer(thread, r, fc, trackVarsArray)
+	addHeadersToServer(thread, r, fc, trackVarsArray)
+	addPreparedEnvToServer(thread, fc, trackVarsArray)
+}
+
+//export go_frankenphp_release_known_variable_keys
+func go_frankenphp_release_known_variable_keys(thread_index C.uintptr_t) {
+	thread := phpThreads[thread_index]
+	if thread.knownVariableKeys == nil {
+		return
+	}
+	for _, v := range thread.knownVariableKeys {
+		C.frankenphp_release_zend_string(v)
+	}
+	// release everything that might still be pinned to the thread
+	thread.Unpin()
+	thread.knownVariableKeys = nil
 }
 
 // splitPos returns the index where path should

frankenphp.c

@@ -9,6 +9,7 @@
 #include <inttypes.h>
 #include <php.h>
 #include <php_config.h>
+#include <php_ini.h>
 #include <php_main.h>
 #include <php_output.h>
 #include <php_variables.h>
@@ -77,6 +78,7 @@ typedef struct frankenphp_server_context {
   bool finished;
 } frankenphp_server_context;
 
+__thread bool should_filter_var = 0;
 __thread frankenphp_server_context *local_ctx = NULL;
 __thread uintptr_t thread_index;
 __thread zval *os_environment = NULL;
@@ -87,25 +89,13 @@ static void frankenphp_free_request_context() {
   free(ctx->cookie_data);
   ctx->cookie_data = NULL;
 
-  free(SG(request_info).auth_password);
+  /* Is freed via thread.Unpin() at the end of each request */
   SG(request_info).auth_password = NULL;
-
-  free(SG(request_info).auth_user);
   SG(request_info).auth_user = NULL;
-
-  free((char *)SG(request_info).request_method);
   SG(request_info).request_method = NULL;
-
-  free(SG(request_info).query_string);
   SG(request_info).query_string = NULL;
-
-  free((char *)SG(request_info).content_type);
   SG(request_info).content_type = NULL;
-
-  free(SG(request_info).path_translated);
   SG(request_info).path_translated = NULL;
-
-  free(SG(request_info).request_uri);
   SG(request_info).request_uri = NULL;
 }
 
@@ -652,106 +642,79 @@ static char *frankenphp_read_cookies(void) {
   return ctx->cookie_data;
 }
 
-static void frankenphp_register_known_variable(const char *key, go_string value,
-                                               zval *track_vars_array) {
-  if (value.data == NULL) {
-    php_register_variable_safe(key, "", 0, track_vars_array);
-    return;
+/* all variables with well defined keys can safely be registered like this */
+void frankenphp_register_trusted_var(zend_string *z_key, char *value,
+                                     int val_len, zval *track_vars_array) {
+  if (value == NULL) {
+    value = "";
   }
-
-  size_t new_val_len;
-  if (sapi_module.input_filter(PARSE_SERVER, key, &value.data, value.len,
-                               &new_val_len)) {
-    php_register_variable_safe(key, value.data, new_val_len, track_vars_array);
+  size_t new_val_len = val_len;
+
+  if (!should_filter_var ||
+      sapi_module.input_filter(PARSE_SERVER, ZSTR_VAL(z_key), &value,
+                               new_val_len, &new_val_len)) {
+    zval z_value;
+    ZVAL_STRINGL_FAST(&z_value, value, new_val_len);
+    zend_hash_update_ind(Z_ARRVAL_P(track_vars_array), z_key, &z_value);
   }
 }
 
+/** Persistent strings are ignored by the PHP GC, we have to release these
+ * ourselves **/
+zend_string *frankenphp_init_persistent_string(const char *string, size_t len) {
+  return zend_string_init(string, len, 1);
+}
+
+void frankenphp_release_zend_string(zend_string *z_string) {
+  zend_string_release(z_string);
+}
+
 static void
-frankenphp_register_variable_from_request_info(const char *key, char *value,
+frankenphp_register_variable_from_request_info(zend_string *zKey, char *value,
+                                               bool must_be_present,
                                                zval *track_vars_array) {
-  if (value == NULL) {
-    return;
-  }
-
-  size_t new_val_len;
-  if (sapi_module.input_filter(PARSE_SERVER, key, &value, strlen(value),
-                               &new_val_len)) {
-    php_register_variable_safe(key, value, new_val_len, track_vars_array);
+  if (value != NULL) {
+    frankenphp_register_trusted_var(zKey, value, strlen(value),
+                                    track_vars_array);
+  } else if (must_be_present) {
+    frankenphp_register_trusted_var(zKey, "", 0, track_vars_array);
   }
 }
 
-void frankenphp_register_bulk_variables(go_string known_variables[27],
-                                        php_variable *dynamic_variables,
-                                        size_t size, zval *track_vars_array) {
-  /* Not used, but must be present */
-  php_register_variable_safe("AUTH_TYPE", "", 0, track_vars_array);
-  php_register_variable_safe("REMOTE_IDENT", "", 0, track_vars_array);
-
-  /* Allocated in frankenphp_update_server_context() */
+void frankenphp_register_variables_from_request_info(
+    zval *track_vars_array, zend_string *content_type,
+    zend_string *path_translated, zend_string *query_string,
+    zend_string *auth_user, zend_string *request_method,
+    zend_string *request_uri) {
   frankenphp_register_variable_from_request_info(
-      "CONTENT_TYPE", (char *)SG(request_info).content_type, track_vars_array);
+      content_type, (char *)SG(request_info).content_type, false,
+      track_vars_array);
   frankenphp_register_variable_from_request_info(
-      "PATH_TRANSLATED", (char *)SG(request_info).path_translated,
+      path_translated, (char *)SG(request_info).path_translated, false,
       track_vars_array);
   frankenphp_register_variable_from_request_info(
-      "QUERY_STRING", SG(request_info).query_string, track_vars_array);
+      query_string, SG(request_info).query_string, true, track_vars_array);
   frankenphp_register_variable_from_request_info(
-      "REMOTE_USER", (char *)SG(request_info).auth_user, track_vars_array);
+      auth_user, (char *)SG(request_info).auth_user, false, track_vars_array);
   frankenphp_register_variable_from_request_info(
-      "REQUEST_METHOD", (char *)SG(request_info).request_method,
+      request_method, (char *)SG(request_info).request_method, false,
       track_vars_array);
   frankenphp_register_variable_from_request_info(
-      "REQUEST_URI", SG(request_info).request_uri, track_vars_array);
-
-  /* Known variables */
-  frankenphp_register_known_variable("CONTENT_LENGTH", known_variables[0],
-                                     track_vars_array);
-  frankenphp_register_known_variable("DOCUMENT_ROOT", known_variables[1],
-                                     track_vars_array);
-  frankenphp_register_known_variable("DOCUMENT_URI", known_variables[2],
-                                     track_vars_array);
-  frankenphp_register_known_variable("GATEWAY_INTERFACE", known_variables[3],
-                                     track_vars_array);
-  frankenphp_register_known_variable("HTTP_HOST", known_variables[4],
-                                     track_vars_array);
-  frankenphp_register_known_variable("HTTPS", known_variables[5],
-                                     track_vars_array);
-  frankenphp_register_known_variable("PATH_INFO", known_variables[6],
-                                     track_vars_array);
-  frankenphp_register_known_variable("PHP_SELF", known_variables[7],
-                                     track_vars_array);
-  frankenphp_register_known_variable("REMOTE_ADDR", known_variables[8],
-                                     track_vars_array);
-  frankenphp_register_known_variable("REMOTE_HOST", known_variables[9],
-                                     track_vars_array);
-  frankenphp_register_known_variable("REMOTE_PORT", known_variables[10],
-                                     track_vars_array);
-  frankenphp_register_known_variable("REQUEST_SCHEME", known_variables[11],
-                                     track_vars_array);
-  frankenphp_register_known_variable("SCRIPT_FILENAME", known_variables[12],
-                                     track_vars_array);
-  frankenphp_register_known_variable("SCRIPT_NAME", known_variables[13],
-                                     track_vars_array);
-  frankenphp_register_known_variable("SERVER_NAME", known_variables[14],
-                                     track_vars_array);
-  frankenphp_register_known_variable("SERVER_PORT", known_variables[15],
-                                     track_vars_array);
-  frankenphp_register_known_variable("SERVER_PROTOCOL", known_variables[16],
-                                     track_vars_array);
-  frankenphp_register_known_variable("SERVER_SOFTWARE", known_variables[17],
-                                     track_vars_array);
-  frankenphp_register_known_variable("SSL_PROTOCOL", known_variables[18],
-                                     track_vars_array);
-
-  size_t new_val_len;
-  for (size_t i = 0; i < size; i++) {
-    if (sapi_module.input_filter(PARSE_SERVER, dynamic_variables[i].var,
-                                 &dynamic_variables[i].data,
-                                 dynamic_variables[i].data_len, &new_val_len)) {
-      php_register_variable_safe(dynamic_variables[i].var,
-                                 dynamic_variables[i].data, new_val_len,
-                                 track_vars_array);
-    }
+      request_uri, SG(request_info).request_uri, true, track_vars_array);
+}
+
+/* variables with user-defined keys must be registered safely
+ * see: php_variables.c -> php_register_variable_ex (#1106) */
+void frankenphp_register_variable_safe(char *key, char *val, size_t val_len,
+                                       zval *track_vars_array) {
+  if (val == NULL || key == NULL) {
+    return;
+  }
+  size_t new_val_len = val_len;
+  if (!should_filter_var ||
+      sapi_module.input_filter(PARSE_SERVER, key, &val, new_val_len,
+                               &new_val_len)) {
+    php_register_variable_safe(key, val, new_val_len, track_vars_array);
   }
 }
 
@@ -854,9 +817,17 @@ static void *php_thread(void *arg) {
 
   local_ctx = malloc(sizeof(frankenphp_server_context));
 
+  /* check if a default filter is set in php.ini and only filter if
+   * it is, this is deprecated and will be removed in PHP 9 */
+  char *default_filter;
+  cfg_get_string("filter.default", &default_filter);
+  should_filter_var = default_filter != NULL;
+
   while (go_handle_request(thread_index)) {
   }
 
+  go_frankenphp_release_known_variable_keys(thread_index);
+
 #ifdef ZTS
   ts_free_thread();
 #endif

frankenphp.go

@@ -393,7 +393,7 @@ func getLogger() *zap.Logger {
 	return logger
 }
 
-func updateServerContext(request *http.Request, create bool, isWorkerRequest bool) error {
+func updateServerContext(thread *phpThread, request *http.Request, create bool, isWorkerRequest bool) error {
 	fc, ok := FromContext(request.Context())
 	if !ok {
 		return InvalidRequestError
@@ -402,28 +402,28 @@ func updateServerContext(request *http.Request, create bool, isWorkerRequest boo
 	authUser, authPassword, ok := request.BasicAuth()
 	var cAuthUser, cAuthPassword *C.char
 	if ok && authPassword != "" {
-		cAuthPassword = C.CString(authPassword)
+		cAuthPassword = thread.pinCString(authPassword)
 	}
 	if ok && authUser != "" {
-		cAuthUser = C.CString(authUser)
+		cAuthUser = thread.pinCString(authUser)
 	}
 
-	cMethod := C.CString(request.Method)
-	cQueryString := C.CString(request.URL.RawQuery)
+	cMethod := thread.pinCString(request.Method)
+	cQueryString := thread.pinCString(request.URL.RawQuery)
 	contentLengthStr := request.Header.Get("Content-Length")
 	contentLength := 0
 	if contentLengthStr != "" {
 		var err error
 		contentLength, err = strconv.Atoi(contentLengthStr)
-		if err != nil {
-			return fmt.Errorf("invalid Content-Length header: %w", err)
+		if err != nil || contentLength < 0 {
+			return fmt.Errorf("Invalid Content-Length header: %w", err)
 		}
 	}
 
 	contentType := request.Header.Get("Content-Type")
 	var cContentType *C.char
 	if contentType != "" {
-		cContentType = C.CString(contentType)
+		cContentType = thread.pinCString(contentType)
 	}
 
 	// compliance with the CGI specification requires that
@@ -431,10 +431,10 @@ func updateServerContext(request *http.Request, create bool, isWorkerRequest boo
 	// Info: https://www.ietf.org/rfc/rfc3875 Page 14
 	var cPathTranslated *C.char
 	if fc.pathInfo != "" {
-		cPathTranslated = C.CString(sanitizedPathJoin(fc.documentRoot, fc.pathInfo)) // Info: http://www.oreilly.com/openbook/cgi/ch02_04.html
+		cPathTranslated = thread.pinCString(sanitizedPathJoin(fc.documentRoot, fc.pathInfo)) // Info: http://www.oreilly.com/openbook/cgi/ch02_04.html
 	}
 
-	cRequestUri := C.CString(request.URL.RequestURI())
+	cRequestUri := thread.pinCString(request.URL.RequestURI())
 	isBootingAWorkerScript := fc.responseWriter == nil
 
 	ret := C.frankenphp_update_server_context(
@@ -462,6 +462,10 @@ func updateServerContext(request *http.Request, create bool, isWorkerRequest boo
 
 // ServeHTTP executes a PHP script according to the given context.
 func ServeHTTP(responseWriter http.ResponseWriter, request *http.Request) error {
+	if !requestIsValid(request, responseWriter) {
+		return nil
+	}
+
 	shutdownWG.Add(1)
 	defer shutdownWG.Done()
 
@@ -595,8 +599,9 @@ func go_handle_request(threadIndex C.uintptr_t) bool {
 			thread.Unpin()
 		}()
 
-		if err := updateServerContext(r, true, false); err != nil {
-			panic(err)
+		if err := updateServerContext(thread, r, true, false); err != nil {
+			rejectRequest(fc.responseWriter, err.Error())
+			return true
 		}
 
 		// scriptFilename is freed in frankenphp_execute_script()
@@ -654,71 +659,6 @@ var headerKeyCache = func() otter.Cache[string, string] {
 	return c
 }()
 
-//export go_register_variables
-func go_register_variables(threadIndex C.uintptr_t, trackVarsArray *C.zval) {
-	thread := phpThreads[threadIndex]
-	r := thread.getActiveRequest()
-	fc := r.Context().Value(contextKey).(*FrankenPHPContext)
-
-	dynamicVariables := make([]C.php_variable, len(fc.env)+len(r.Header))
-
-	var l int
-
-	// Add all HTTP headers to env variables
-	for field, val := range r.Header {
-		k, ok := headerKeyCache.Get(field)
-		if !ok {
-			k = "HTTP_" + headerNameReplacer.Replace(strings.ToUpper(field)) + "\x00"
-			headerKeyCache.SetIfAbsent(field, k)
-		}
-
-		if _, ok := fc.env[k]; ok {
-			continue
-		}
-
-		v := strings.Join(val, ", ")
-
-		kData := unsafe.StringData(k)
-		vData := unsafe.StringData(v)
-
-		thread.Pin(kData)
-		thread.Pin(vData)
-
-		dynamicVariables[l]._var = (*C.char)(unsafe.Pointer(kData))
-		dynamicVariables[l].data_len = C.size_t(len(v))
-		dynamicVariables[l].data = (*C.char)(unsafe.Pointer(vData))
-
-		l++
-	}
-
-	for k, v := range fc.env {
-		if _, ok := knownServerKeys[k]; ok {
-			continue
-		}
-
-		kData := unsafe.StringData(k)
-		vData := unsafe.Pointer(unsafe.StringData(v))
-
-		thread.Pin(kData)
-		thread.Pin(vData)
-
-		dynamicVariables[l]._var = (*C.char)(unsafe.Pointer(kData))
-		dynamicVariables[l].data_len = C.size_t(len(v))
-		dynamicVariables[l].data = (*C.char)(unsafe.Pointer(vData))
-
-		l++
-	}
-
-	knownVariables := computeKnownVariables(r, &thread.Pinner)
-
-	dvsd := unsafe.SliceData(dynamicVariables)
-	thread.Pin(dvsd)
-
-	C.frankenphp_register_bulk_variables(&knownVariables[0], dvsd, C.size_t(l), trackVarsArray)
-
-	fc.env = nil
-}
-
 //export go_apache_request_headers
 func go_apache_request_headers(threadIndex C.uintptr_t, hasActiveRequest bool) (*C.go_string, C.size_t) {
 	thread := phpThreads[threadIndex]
@@ -927,3 +867,18 @@ func executePHPFunction(functionName string) {
 		}
 	}
 }
+
+// Ensure that the request path does not contain null bytes
+func requestIsValid(r *http.Request, rw http.ResponseWriter) bool {
+	if !strings.Contains(r.URL.Path, "\x00") {
+		return true
+	}
+	rejectRequest(rw, "Invalid request path")
+	return false
+}
+
+func rejectRequest(rw http.ResponseWriter, message string) {
+	rw.WriteHeader(http.StatusBadRequest)
+	_, _ = rw.Write([]byte(message))
+	rw.(http.Flusher).Flush()
+}

frankenphp.h

@@ -50,12 +50,21 @@ int frankenphp_update_server_context(
     char *auth_user, char *auth_password, int proto_num);
 int frankenphp_request_startup();
 int frankenphp_execute_script(char *file_name);
-void frankenphp_register_bulk_variables(go_string known_variables[27],
-                                        php_variable *dynamic_variables,
-                                        size_t size, zval *track_vars_array);
 
 int frankenphp_execute_script_cli(char *script, int argc, char **argv);
 
 int frankenphp_execute_php_function(const char *php_function);
 
+void frankenphp_register_variables_from_request_info(
+    zval *track_vars_array, zend_string *content_type,
+    zend_string *path_translated, zend_string *query_string,
+    zend_string *auth_user, zend_string *request_method,
+    zend_string *request_uri);
+void frankenphp_register_variable_safe(char *key, char *var, size_t val_len,
+                                       zval *track_vars_array);
+void frankenphp_register_trusted_var(zend_string *z_key, char *value,
+                                     int val_len, zval *track_vars_array);
+zend_string *frankenphp_init_persistent_string(const char *string, size_t len);
+void frankenphp_release_zend_string(zend_string *z_string);
+
 #endif

frankenphp_test.go

@@ -19,6 +19,7 @@ import (
 	"net/url"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"strconv"
 	"strings"
 	"sync"
@@ -888,3 +889,72 @@ func BenchmarkServerSuperGlobal(b *testing.B) {
 		handler(w, req)
 	}
 }
+
+func TestRejectInvalidHeaders_module(t *testing.T) { testRejectInvalidHeaders(t, &testOptions{}) }
+func TestRejectInvalidHeaders_worker(t *testing.T) {
+	testRejectInvalidHeaders(t, &testOptions{workerScript: "headers.php"})
+}
+func testRejectInvalidHeaders(t *testing.T, opts *testOptions) {
+	invalidHeaders := [][]string{
+		[]string{"Content-Length", "-1"},
+		[]string{"Content-Length", "something"},
+	}
+	for _, header := range invalidHeaders {
+		runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, _ int) {
+			req := httptest.NewRequest("GET", "http://example.com/headers.php", nil)
+			req.Header.Add(header[0], header[1])
+
+			w := httptest.NewRecorder()
+			handler(w, req)
+
+			resp := w.Result()
+			body, _ := io.ReadAll(resp.Body)
+
+			assert.Equal(t, 400, resp.StatusCode)
+			assert.Contains(t, string(body), "Invalid")
+		}, opts)
+	}
+}
+
+// To run this fuzzing test use: go test -fuzz FuzzRequest
+// TODO: Cover more potential cases
+func FuzzRequest(f *testing.F) {
+	f.Add("hello world")
+	f.Add("😀😅🙃🤩🥲🤪😘😇😉🐘🧟")
+	f.Add("%00%11%%22%%33%%44%%55%%66%%77%%88%%99%%aa%%bb%%cc%%dd%%ee%%ff")
+	f.Add("\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f")
+	f.Fuzz(func(t *testing.T, fuzzedString string) {
+		absPath, _ := filepath.Abs("./testdata/")
+		runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, _ int) {
+			req := httptest.NewRequest("GET", "http://example.com/server-variable", nil)
+			req.URL = &url.URL{RawQuery: "test=" + fuzzedString, Path: "/server-variable.php/" + fuzzedString}
+			req.Header.Add(strings.Clone("Fuzzed"), strings.Clone(fuzzedString))
+			req.Header.Add(strings.Clone("Content-Type"), fuzzedString)
+
+			w := httptest.NewRecorder()
+			handler(w, req)
+
+			resp := w.Result()
+			body, _ := io.ReadAll(resp.Body)
+
+			// The response status must be 400 if the request path contains null bytes
+			if strings.Contains(req.URL.Path, "\x00") {
+				assert.Equal(t, 400, resp.StatusCode)
+				assert.Contains(t, string(body), "Invalid request path")
+				return
+			}
+
+			// The fuzzed string must be present in the path
+			assert.Contains(t, string(body), fmt.Sprintf("[PATH_INFO] => /%s", fuzzedString))
+			assert.Contains(t, string(body), fmt.Sprintf("[PATH_TRANSLATED] => %s", filepath.Join(absPath, fuzzedString)))
+
+			// The header should only be present if the fuzzed string is not empty
+			if len(fuzzedString) > 0 {
+				assert.Contains(t, string(body), fmt.Sprintf("[CONTENT_TYPE] => %s", fuzzedString))
+				assert.Contains(t, string(body), fmt.Sprintf("[HTTP_FUZZED] => %s", fuzzedString))
+			} else {
+				assert.NotContains(t, string(body), "[HTTP_FUZZED]")
+			}
+		}, &testOptions{workerScript: "request-headers.php"})
+	})
+}

php_thread.go

@@ -1,10 +1,12 @@
 package frankenphp
 
 // #include <stdint.h>
+// #include <php_variables.h>
 import "C"
 import (
 	"net/http"
 	"runtime"
+	"unsafe"
 )
 
 var phpThreads []*phpThread
@@ -12,9 +14,10 @@ var phpThreads []*phpThread
 type phpThread struct {
 	runtime.Pinner
 
-	mainRequest   *http.Request
-	workerRequest *http.Request
-	worker        *worker
+	mainRequest       *http.Request
+	workerRequest     *http.Request
+	worker            *worker
+	knownVariableKeys map[string]*C.zend_string
 }
 
 func initPHPThreads(numThreads int) {
@@ -31,3 +34,16 @@ func (thread phpThread) getActiveRequest() *http.Request {
 
 	return thread.mainRequest
 }
+
+// Pin a string that is not null-terminated
+// PHP's zend_string may contain null-bytes
+func (thread *phpThread) pinString(s string) *C.char {
+	sData := unsafe.StringData(s)
+	thread.Pin(sData)
+	return (*C.char)(unsafe.Pointer(sData))
+}
+
+// C strings must be null-terminated
+func (thread *phpThread) pinCString(s string) *C.char {
+	return thread.pinString(s+"\x00")
+}

testdata/server-all-vars-ordered.php

@@ -0,0 +1,39 @@
+<?php
+
+echo "<pre>\n";
+foreach ([
+             'CONTENT_LENGTH',
+             'HTTP_CONTENT_LENGTH',
+             'HTTP_SPECIAL_CHARS',
+             'DOCUMENT_ROOT',
+             'DOCUMENT_URI',
+             'GATEWAY_INTERFACE',
+             'HTTP_HOST',
+             'HTTPS',
+             'PATH_INFO',
+             'CONTENT_TYPE',
+             'DOCUMENT_ROOT',
+             'REMOTE_ADDR',
+             'CONTENT_LENGTH',
+             'PHP_SELF',
+             'REMOTE_HOST',
+             'REQUEST_SCHEME',
+             'SCRIPT_FILENAME',
+             'SCRIPT_NAME',
+             'SERVER_NAME',
+             'SERVER_PORT',
+             'SERVER_PROTOCOL',
+             'SERVER_SOFTWARE',
+             'SSL_PROTOCOL',
+             'AUTH_TYPE',
+             'REMOTE_IDENT',
+             'CONTENT_TYPE',
+             'PATH_TRANSLATED',
+             'QUERY_STRING',
+             'REMOTE_USER',
+             'REQUEST_METHOD',
+             'REQUEST_URI',
+         ] as $name) {
+    echo "$name:" . $_SERVER[$name] . "\n";
+}
+echo "</pre>";

testdata/server-all-vars-ordered.txt

@@ -0,0 +1,33 @@
+<pre>
+CONTENT_LENGTH:7
+HTTP_CONTENT_LENGTH:7
+HTTP_SPECIAL_CHARS:<%00>
+DOCUMENT_ROOT:{documentRoot}
+DOCUMENT_URI:/server-all-vars-ordered.php
+GATEWAY_INTERFACE:CGI/1.1
+HTTP_HOST:localhost:{testPort}
+HTTPS:
+PATH_INFO:/path
+CONTENT_TYPE:application/x-www-form-urlencoded
+DOCUMENT_ROOT:{documentRoot}
+REMOTE_ADDR:127.0.0.1
+CONTENT_LENGTH:7
+PHP_SELF:/server-all-vars-ordered.php/path
+REMOTE_HOST:127.0.0.1
+REQUEST_SCHEME:http
+SCRIPT_FILENAME:{documentRoot}/server-all-vars-ordered.php
+SCRIPT_NAME:/server-all-vars-ordered.php
+SERVER_NAME:localhost
+SERVER_PORT:{testPort}
+SERVER_PROTOCOL:HTTP/1.1
+SERVER_SOFTWARE:FrankenPHP
+SSL_PROTOCOL:
+AUTH_TYPE:
+REMOTE_IDENT:
+CONTENT_TYPE:application/x-www-form-urlencoded
+PATH_TRANSLATED:{documentRoot}/path
+QUERY_STRING:specialChars=%3E\x00%00</>
+REMOTE_USER:user
+REQUEST_METHOD:POST
+REQUEST_URI:/server-all-vars-ordered.php/path?specialChars=%3E\x00%00</>
+</pre>