fix(worker): initialize $_RESUEST (#2136)

Hi,

This PR fixes #1931, it handles $_REQUEST in worker mode correctly when
`auto_globals_jit` is enabled (default configuration for PHP).
Some concerns were raised in the comments of the issue regarding
performance. This implementation should make sure that request is
created only if used.

However if a previous execution plan already used `_REQUEST`, all
subsequent requests will create it. So the concern is "kindof"
mitigated.

Let me know if you have any suggestion to improve this.

---------

Signed-off-by: Xavier Leune <[email protected]>
Co-authored-by: Alexander Stecher <[email protected]>

Author: xavierleune

frankenphp.c

@@ -128,13 +128,19 @@ static void frankenphp_reset_super_globals() {
     if (auto_global->name == _env) {
       /* skip $_ENV */
     } else if (auto_global->name == _server) {
-      /* always reimport $_SERVER  */
+      /* always reimport $_SERVER */
       auto_global->armed = auto_global->auto_global_callback(auto_global->name);
     } else if (auto_global->jit) {
-      /* globals with jit are: $_SERVER, $_ENV, $_REQUEST, $GLOBALS,
-       * jit will only trigger on script parsing and therefore behaves
-       * differently in worker mode. We will skip all jit globals
-       */
+      /* JIT globals ($_REQUEST, $GLOBALS) need special handling:
+       * - $GLOBALS will always be handled by the application, we skip it
+       * For $_REQUEST:
+       * - If in symbol_table: re-initialize with current request data
+       * - If not: do nothing, it may be armed by jit later */
+      if (auto_global->name == ZSTR_KNOWN(ZEND_STR_AUTOGLOBAL_REQUEST) &&
+          zend_hash_exists(&EG(symbol_table), auto_global->name)) {
+        auto_global->armed =
+            auto_global->auto_global_callback(auto_global->name);
+      }
     } else if (auto_global->auto_global_callback) {
       /* $_GET, $_POST, $_COOKIE, $_FILES are reimported here */
       auto_global->armed = auto_global->auto_global_callback(auto_global->name);

frankenphp_test.go

@@ -306,6 +306,56 @@ func testPostSuperGlobals(t *testing.T, opts *testOptions) {
 	}, opts)
 }
 
+func TestRequestSuperGlobal_module(t *testing.T) { testRequestSuperGlobal(t, nil) }
+func TestRequestSuperGlobal_worker(t *testing.T) {
+	phpIni := make(map[string]string)
+	phpIni["auto_globals_jit"] = "1"
+	testRequestSuperGlobal(t, &testOptions{workerScript: "request-superglobal.php", phpIni: phpIni})
+}
+func testRequestSuperGlobal(t *testing.T, opts *testOptions) {
+	runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, i int) {
+		// Test with both GET and POST parameters
+		// $_REQUEST should contain merged data from both
+		formData := url.Values{"post_key": {fmt.Sprintf("post_value_%d", i)}}
+		req := httptest.NewRequest("POST", fmt.Sprintf("http://example.com/request-superglobal.php?get_key=get_value_%d", i), strings.NewReader(formData.Encode()))
+		req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+		body, _ := testRequest(req, handler, t)
+
+		// Verify $_REQUEST contains both GET and POST data for the current request
+		assert.Contains(t, body, fmt.Sprintf("'get_key' => 'get_value_%d'", i))
+		assert.Contains(t, body, fmt.Sprintf("'post_key' => 'post_value_%d'", i))
+	}, opts)
+}
+
+func TestRequestSuperGlobalConditional_worker(t *testing.T) {
+	// This test verifies that $_REQUEST works correctly when accessed conditionally
+	// in worker mode. The first request does NOT access $_REQUEST, but subsequent
+	// requests do. This tests the "re-arm" mechanism for JIT auto globals.
+	//
+	// The bug scenario:
+	// - Request 1 (i=1): includes file, $_REQUEST initialized with val=1
+	// - Request 3 (i=3): includes file from cache, $_REQUEST should have val=3
+	// If the bug exists, $_REQUEST would still have val=1 from request 1.
+	phpIni := make(map[string]string)
+	phpIni["auto_globals_jit"] = "1"
+	runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, i int) {
+		if i%2 == 0 {
+			// Even requests: don't use $_REQUEST
+			body, _:= testGet(fmt.Sprintf("http://example.com/request-superglobal-conditional.php?val=%d", i), handler, t)
+			assert.Contains(t, body, "SKIPPED")
+			assert.Contains(t, body, fmt.Sprintf("'val' => '%d'", i))
+		} else {
+			// Odd requests: use $_REQUEST
+			body, _ := testGet(fmt.Sprintf("http://example.com/request-superglobal-conditional.php?use_request=1&val=%d", i), handler, t)
+			assert.Contains(t, body, "REQUEST:")
+			assert.Contains(t, body, "REQUEST_COUNT:2", "$_REQUEST should have ONLY current request's data (2 keys: use_request and val)")
+			assert.Contains(t, body, fmt.Sprintf("'val' => '%d'", i), "request data is not present")
+			assert.Contains(t, body, "'use_request' => '1'")
+			assert.Contains(t, body, "VAL_CHECK:MATCH", "BUG: $_REQUEST contains stale data from previous request! Body: "+body)
+		}
+	}, &testOptions{workerScript: "request-superglobal-conditional.php", phpIni: phpIni})
+}
+
 func TestCookies_module(t *testing.T) { testCookies(t, nil) }
 func TestCookies_worker(t *testing.T) { testCookies(t, &testOptions{workerScript: "cookies.php"}) }
 func testCookies(t *testing.T, opts *testOptions) {

testdata/request-superglobal-conditional-include.php

@@ -0,0 +1,20 @@
+<?php
+// This file tests if $_REQUEST is properly re-initialized in worker mode
+// The key test is: does $_REQUEST contain ONLY the current request's data?
+
+// Static counter to track how many times this file is executed (not compiled)
+static $execCount = 0;
+$execCount++;
+
+echo "EXEC_COUNT:" . $execCount;
+echo "\nREQUEST:";
+var_export($_REQUEST);
+echo "\nREQUEST_COUNT:" . count($_REQUEST);
+
+// Check if $_REQUEST was properly initialized for this request
+// If stale, it might have data from a previous request
+if (isset($_GET['val'])) {
+    $expected_val = $_GET['val'];
+    $actual_val = $_REQUEST['val'] ?? 'MISSING';
+    echo "\nVAL_CHECK:" . ($expected_val === $actual_val ? "MATCH" : "MISMATCH(expected=$expected_val,actual=$actual_val)");
+}

testdata/request-superglobal-conditional.php

@@ -0,0 +1,16 @@
+<?php
+
+require_once __DIR__.'/_executor.php';
+
+return function () {
+    // Only access $_REQUEST on requests where use_request=1 is passed
+    // This tests the "re-arm" scenario where $_REQUEST might be accessed
+    // for the first time during a later request
+    if (isset($_GET['use_request']) && $_GET['use_request'] === '1') {
+        include 'request-superglobal-conditional-include.php';
+    } else {
+        echo "SKIPPED";
+    }
+    echo "\nGET:";
+    var_export($_GET);
+};

testdata/request-superglobal.php

@@ -0,0 +1,14 @@
+<?php
+
+require_once __DIR__.'/_executor.php';
+
+return function () {
+    // Output $_REQUEST to verify it contains current request data
+    // $_REQUEST should be a merge of $_GET and $_POST (based on request_order)
+    echo "REQUEST:";
+    var_export($_REQUEST);
+    echo "\nGET:";
+    var_export($_GET);
+    echo "\nPOST:";
+    var_export($_POST);
+};