Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: Put all $_SERVER vars into one function call. #1303

Merged
merged 10 commits into from
Jan 8, 2025
Merged

perf: Put all $_SERVER vars into one function call. #1303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
411a8fc
Puts everything into one function call.
Alliballibaba2 Jan 5, 2025
e4c305d
Clang-format off.
Alliballibaba2 Jan 5, 2025
fff8153
Merge branch 'main' into perf/reduce-cgo-calls
Alliballibaba2 Jan 6, 2025
77ac74d
Cleans up.
Alliballibaba2 Jan 6, 2025
85604a6
Changes function name.
Alliballibaba2 Jan 6, 2025
80d00ca
Removes unnecessary check.
Alliballibaba2 Jan 6, 2025
eb556ad
Passes hash table directly.
Alliballibaba2 Jan 6, 2025
f280107
Also tests that the original request path is passed.
Alliballibaba2 Jan 7, 2025
9f9f18f
Puts vars into a struct.
Alliballibaba2 Jan 7, 2025
fc405dc
clang-format
Alliballibaba2 Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions caddy/caddy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,13 +301,11 @@ func (f FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ ca
documentRootOption = frankenphp.WithRequestResolvedDocumentRoot(f.resolvedDocumentRoot)
}

env := make(map[string]string, len(f.preparedEnv)+1)
env["REQUEST_URI\x00"] = origReq.URL.RequestURI()
for k, v := range f.preparedEnv {
if f.preparedEnvNeedsReplacement {
env := f.preparedEnv
if f.preparedEnvNeedsReplacement {
env = make(frankenphp.PreparedEnv, len(f.Env))
for k, v := range f.preparedEnv {
env[k] = repl.ReplaceKnown(v, "")
} else {
env[k] = v
}
}

Expand All @@ -316,6 +314,7 @@ func (f FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ ca
documentRootOption,
frankenphp.WithRequestSplitPath(f.SplitPath),
frankenphp.WithRequestPreparedEnv(env),
frankenphp.WithOriginalRequest(&origReq),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we keep the previous strategy to not increase the public API surface?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try passing this via context instead. Doing it via the prepared env requires copying and registering it on every request (and passing the request uri via environment is a bit confusing)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, using the context is a good idea.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized that you actually can't pass via context between packages, so this would need to be in the public api

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's necessarily bad to have this in a public api since having a different 'actual' request uri is a key PHP behavior for any application using index files. We can also only pass the uri instead of the whole request.

If you still dislike that, we can also reset it back to be passed via env (feels hacky but minimizes the public api)

)

if err != nil {
Expand Down
4 changes: 3 additions & 1 deletion caddy/caddy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,12 +596,14 @@ func TestAllDefinedServerVars(t *testing.T) {
localhost:`+testPort+` {
route {
root ../testdata
# rewrite to test that the original path is passed as $REQUEST_URI
rewrite /server-all-vars-ordered.php/path
php
}
}
`, "caddyfile")
tester.AssertPostResponseBody(
"http://user@localhost:"+testPort+"/server-all-vars-ordered.php/path?specialChars=%3E\\x00%00</>",
"http://user@localhost:"+testPort+"/original-path?specialChars=%3E\\x00%00</>",
[]string{
"Content-Type: application/x-www-form-urlencoded",
"Content-Length: 14", // maliciously set to 14
Expand Down
180 changes: 83 additions & 97 deletions cgi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,34 +12,34 @@ import (
"unsafe"
)

var knownServerKeys = map[string]struct{}{
"CONTENT_LENGTH\x00": {},
"DOCUMENT_ROOT\x00": {},
"DOCUMENT_URI\x00": {},
"GATEWAY_INTERFACE\x00": {},
"HTTP_HOST\x00": {},
"HTTPS\x00": {},
"PATH_INFO\x00": {},
"PHP_SELF\x00": {},
"REMOTE_ADDR\x00": {},
"REMOTE_HOST\x00": {},
"REMOTE_PORT\x00": {},
"REQUEST_SCHEME\x00": {},
"SCRIPT_FILENAME\x00": {},
"SCRIPT_NAME\x00": {},
"SERVER_NAME\x00": {},
"SERVER_PORT\x00": {},
"SERVER_PROTOCOL\x00": {},
"SERVER_SOFTWARE\x00": {},
"SSL_PROTOCOL\x00": {},
"AUTH_TYPE\x00": {},
"REMOTE_IDENT\x00": {},
"CONTENT_TYPE\x00": {},
"PATH_TRANSLATED\x00": {},
"QUERY_STRING\x00": {},
"REMOTE_USER\x00": {},
"REQUEST_METHOD\x00": {},
"REQUEST_URI\x00": {},
var knownServerKeys = []string{
"CONTENT_LENGTH",
"DOCUMENT_ROOT",
"DOCUMENT_URI",
"GATEWAY_INTERFACE",
"HTTP_HOST",
"HTTPS",
"PATH_INFO",
"PHP_SELF",
"REMOTE_ADDR",
"REMOTE_HOST",
"REMOTE_PORT",
"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",
}

// computeKnownVariables returns a set of CGI environment variables for the request.
Expand All @@ -61,58 +61,26 @@ func addKnownVariablesToServer(thread *phpThread, request *http.Request, fc *Fra
ip = strings.Replace(ip, "[", "", 1)
ip = strings.Replace(ip, "]", "", 1)

ra, raOK := fc.env["REMOTE_ADDR\x00"]
if raOK {
registerTrustedVar(keys["REMOTE_ADDR\x00"], ra, trackVarsArray)
} else {
registerTrustedVar(keys["REMOTE_ADDR\x00"], ip, trackVarsArray)
}

if rh, ok := fc.env["REMOTE_HOST\x00"]; ok {
registerTrustedVar(keys["REMOTE_HOST\x00"], rh, trackVarsArray) // For speed, remote host lookups disabled
} else {
if raOK {
registerTrustedVar(keys["REMOTE_HOST\x00"], ra, trackVarsArray)
} else {
registerTrustedVar(keys["REMOTE_HOST\x00"], ip, trackVarsArray)
}
}

registerTrustedVar(keys["REMOTE_PORT\x00"], port, trackVarsArray)
registerTrustedVar(keys["DOCUMENT_ROOT\x00"], fc.documentRoot, trackVarsArray)
registerTrustedVar(keys["PATH_INFO\x00"], fc.pathInfo, trackVarsArray)
registerTrustedVar(keys["PHP_SELF\x00"], request.URL.Path, trackVarsArray)
registerTrustedVar(keys["DOCUMENT_URI\x00"], fc.docURI, trackVarsArray)
registerTrustedVar(keys["SCRIPT_FILENAME\x00"], fc.scriptFilename, trackVarsArray)
registerTrustedVar(keys["SCRIPT_NAME\x00"], fc.scriptName, trackVarsArray)

var https string
var sslProtocol string
var rs string
if request.TLS == nil {
rs = "http"
registerTrustedVar(keys["HTTPS\x00"], "", trackVarsArray)
registerTrustedVar(keys["SSL_PROTOCOL\x00"], "", trackVarsArray)
https = ""
sslProtocol = ""
} else {
rs = "https"
if h, ok := fc.env["HTTPS\x00"]; ok {
registerTrustedVar(keys["HTTPS\x00"], h, trackVarsArray)
} else {
registerTrustedVar(keys["HTTPS\x00"], "on", trackVarsArray)
}
https = "on"

// and pass the protocol details in a manner compatible with apache's mod_ssl
// (which is why these have an SSL_ prefix and not TLS_).
if pr, ok := fc.env["SSL_PROTOCOL\x00"]; ok {
registerTrustedVar(keys["SSL_PROTOCOL\x00"], pr, trackVarsArray)
if v, ok := tlsProtocolStrings[request.TLS.Version]; ok {
sslProtocol = v
} else {
if v, ok := tlsProtocolStrings[request.TLS.Version]; ok {
registerTrustedVar(keys["SSL_PROTOCOL\x00"], v, trackVarsArray)
} else {
registerTrustedVar(keys["SSL_PROTOCOL\x00"], "", trackVarsArray)
}
sslProtocol = ""
}
}

registerTrustedVar(keys["REQUEST_SCHEME\x00"], rs, trackVarsArray)
reqHost, reqPort, _ := net.SplitHostPort(request.Host)

if reqHost == "" {
Expand All @@ -133,42 +101,61 @@ func addKnownVariablesToServer(thread *phpThread, request *http.Request, fc *Fra
}
}

registerTrustedVar(keys["SERVER_NAME\x00"], reqHost, trackVarsArray)
if reqPort != "" {
registerTrustedVar(keys["SERVER_PORT\x00"], reqPort, trackVarsArray)
serverPort := reqPort
contentLength := request.Header.Get("Content-Length")

var requestURI string
if fc.originalRequest != nil {
requestURI = fc.originalRequest.URL.RequestURI()
} else {
registerTrustedVar(keys["SERVER_PORT\x00"], "", trackVarsArray)
requestURI = request.URL.RequestURI()
}

// 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 overridden
registerTrustedVar(keys["CONTENT_LENGTH\x00"], request.Header.Get("Content-Length"), trackVarsArray)
registerTrustedVar(keys["GATEWAY_INTERFACE\x00"], "CGI/1.1", trackVarsArray)
registerTrustedVar(keys["SERVER_PROTOCOL\x00"], request.Proto, trackVarsArray)
registerTrustedVar(keys["SERVER_SOFTWARE\x00"], "FrankenPHP", trackVarsArray)
registerTrustedVar(keys["HTTP_HOST\x00"], request.Host, trackVarsArray) // added here, since not always part of headers

// These values are always empty but must be defined:
registerTrustedVar(keys["AUTH_TYPE\x00"], "", trackVarsArray)
registerTrustedVar(keys["REMOTE_IDENT\x00"], "", trackVarsArray)
C.frankenphp_register_bulk(
trackVarsArray,
packCgiVariable(keys["REMOTE_ADDR"], ip),
packCgiVariable(keys["REMOTE_HOST"], ip),
packCgiVariable(keys["REMOTE_PORT"], port),
packCgiVariable(keys["DOCUMENT_ROOT"], fc.documentRoot),
packCgiVariable(keys["PATH_INFO"], fc.pathInfo),
packCgiVariable(keys["PHP_SELF"], request.URL.Path),
packCgiVariable(keys["DOCUMENT_URI"], fc.docURI),
packCgiVariable(keys["SCRIPT_FILENAME"], fc.scriptFilename),
packCgiVariable(keys["SCRIPT_NAME"], fc.scriptName),
packCgiVariable(keys["HTTPS"], https),
packCgiVariable(keys["SSL_PROTOCOL"], sslProtocol),
packCgiVariable(keys["REQUEST_SCHEME"], rs),
packCgiVariable(keys["SERVER_NAME"], reqHost),
packCgiVariable(keys["SERVER_PORT"], serverPort),
// 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 overridden
packCgiVariable(keys["CONTENT_LENGTH"], contentLength),
packCgiVariable(keys["GATEWAY_INTERFACE"], "CGI/1.1"),
packCgiVariable(keys["SERVER_PROTOCOL"], request.Proto),
packCgiVariable(keys["SERVER_SOFTWARE"], "FrankenPHP"),
packCgiVariable(keys["HTTP_HOST"], request.Host),
// These values are always empty but must be defined:
packCgiVariable(keys["AUTH_TYPE"], ""),
packCgiVariable(keys["REMOTE_IDENT"], ""),
// Request uri of the original request
packCgiVariable(keys["REQUEST_URI"], requestURI),
)

// These values are already present in the SG(request_info), so we'll register them from there
C.frankenphp_register_variables_from_request_info(
trackVarsArray,
keys["CONTENT_TYPE\x00"],
keys["PATH_TRANSLATED\x00"],
keys["QUERY_STRING\x00"],
keys["REMOTE_USER\x00"],
keys["REQUEST_METHOD\x00"],
keys["REQUEST_URI\x00"],
keys["CONTENT_TYPE"],
keys["PATH_TRANSLATED"],
keys["QUERY_STRING"],
keys["REMOTE_USER"],
keys["REQUEST_METHOD"],
)
}

func registerTrustedVar(key *C.zend_string, value string, trackVarsArray *C.zval) {
C.frankenphp_register_trusted_var(key, toUnsafeChar(value), C.int(len(value)), trackVarsArray)
func packCgiVariable(key *C.zend_string, value string) C.ht_key_value_pair {
return C.ht_key_value_pair{key, toUnsafeChar(value), C.size_t(len(value))}
}

func addHeadersToServer(request *http.Request, fc *FrankenPHPContext, trackVarsArray *C.zval) {
Expand Down Expand Up @@ -200,9 +187,8 @@ func getKnownVariableKeys(thread *phpThread) map[string]*C.zend_string {
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(toUnsafeChar(keyWithoutNull), C.size_t(len(keyWithoutNull)))
for _, k := range knownServerKeys {
threadServerKeys[k] = C.frankenphp_init_persistent_string(toUnsafeChar(k), C.size_t(len(k)))
}
thread.knownVariableKeys = threadServerKeys
return threadServerKeys
Expand Down
78 changes: 69 additions & 9 deletions frankenphp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,9 +644,12 @@ static char *frankenphp_read_cookies(void) {

/* 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) {
size_t val_len, HashTable *ht) {
if (value == NULL) {
value = "";
zval empty;
ZVAL_EMPTY_STRING(&empty);
zend_hash_update_ind(ht, z_key, &empty);
return;
}
size_t new_val_len = val_len;

Expand All @@ -655,10 +658,70 @@ void frankenphp_register_trusted_var(zend_string *z_key, char *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);
zend_hash_update_ind(ht, z_key, &z_value);
}
}

/* Register known $_SERVER variables in bulk to avoid cgo overhead */
void frankenphp_register_bulk(
zval *track_vars_array, ht_key_value_pair remote_addr,
ht_key_value_pair remote_host, ht_key_value_pair remote_port,
ht_key_value_pair document_root, ht_key_value_pair path_info,
ht_key_value_pair php_self, ht_key_value_pair document_uri,
ht_key_value_pair script_filename, ht_key_value_pair script_name,
ht_key_value_pair https, ht_key_value_pair ssl_protocol,
ht_key_value_pair request_scheme, ht_key_value_pair server_name,
ht_key_value_pair server_port, ht_key_value_pair content_length,
ht_key_value_pair gateway_interface, ht_key_value_pair server_protocol,
ht_key_value_pair server_software, ht_key_value_pair http_host,
ht_key_value_pair auth_type, ht_key_value_pair remote_ident,
ht_key_value_pair request_uri) {
HashTable *ht = Z_ARRVAL_P(track_vars_array);
frankenphp_register_trusted_var(remote_addr.key, remote_addr.val,
remote_addr.val_len, ht);
frankenphp_register_trusted_var(remote_host.key, remote_host.val,
remote_host.val_len, ht);
frankenphp_register_trusted_var(remote_port.key, remote_port.val,
remote_port.val_len, ht);
frankenphp_register_trusted_var(document_root.key, document_root.val,
document_root.val_len, ht);
frankenphp_register_trusted_var(path_info.key, path_info.val,
path_info.val_len, ht);
frankenphp_register_trusted_var(php_self.key, php_self.val, php_self.val_len,
ht);
frankenphp_register_trusted_var(document_uri.key, document_uri.val,
document_uri.val_len, ht);
frankenphp_register_trusted_var(script_filename.key, script_filename.val,
script_filename.val_len, ht);
frankenphp_register_trusted_var(script_name.key, script_name.val,
script_name.val_len, ht);
frankenphp_register_trusted_var(https.key, https.val, https.val_len, ht);
frankenphp_register_trusted_var(ssl_protocol.key, ssl_protocol.val,
ssl_protocol.val_len, ht);
frankenphp_register_trusted_var(request_scheme.key, request_scheme.val,
request_scheme.val_len, ht);
frankenphp_register_trusted_var(server_name.key, server_name.val,
server_name.val_len, ht);
frankenphp_register_trusted_var(server_port.key, server_port.val,
server_port.val_len, ht);
frankenphp_register_trusted_var(content_length.key, content_length.val,
content_length.val_len, ht);
frankenphp_register_trusted_var(gateway_interface.key, gateway_interface.val,
gateway_interface.val_len, ht);
frankenphp_register_trusted_var(server_protocol.key, server_protocol.val,
server_protocol.val_len, ht);
frankenphp_register_trusted_var(server_software.key, server_software.val,
server_software.val_len, ht);
frankenphp_register_trusted_var(http_host.key, http_host.val,
http_host.val_len, ht);
frankenphp_register_trusted_var(auth_type.key, auth_type.val,
auth_type.val_len, ht);
frankenphp_register_trusted_var(remote_ident.key, remote_ident.val,
remote_ident.val_len, ht);
frankenphp_register_trusted_var(request_uri.key, request_uri.val,
request_uri.val_len, ht);
}

/** 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) {
Expand All @@ -675,17 +738,16 @@ frankenphp_register_variable_from_request_info(zend_string *zKey, char *value,
zval *track_vars_array) {
if (value != NULL) {
frankenphp_register_trusted_var(zKey, value, strlen(value),
track_vars_array);
Z_ARRVAL_P(track_vars_array));
} else if (must_be_present) {
frankenphp_register_trusted_var(zKey, "", 0, track_vars_array);
frankenphp_register_trusted_var(zKey, "", 0, Z_ARRVAL_P(track_vars_array));
}
}

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) {
zend_string *auth_user, zend_string *request_method) {
frankenphp_register_variable_from_request_info(
content_type, (char *)SG(request_info).content_type, false,
track_vars_array);
Expand All @@ -699,8 +761,6 @@ void frankenphp_register_variables_from_request_info(
frankenphp_register_variable_from_request_info(
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, true, track_vars_array);
}

/* variables with user-defined keys must be registered safely
Expand Down
Loading
Loading