Skip to content

Commit d82fc4f

Browse files
perf: avoid copying already rooted request paths (#2794)
Preparing SCRIPT_FILENAME currently prepends `/` even when the request path already starts with it. The resulting extra slash makes `path.Clean` rebuild an otherwise clean path. Only prepend `/` for unrooted paths. Keep path cleaning, the Windows locality check, and trailing-slash handling. Add cases for repeated leading slashes and traversal, plus a benchmark for rooted, relative, and traversal paths. Earlier standalone measurements of this helper (Go 1.27.3, darwin/arm64, median of five 200ms samples): | Path | Before | After | Time reduction | Go B/op | allocs/op | | --- | ---: | ---: | ---: | ---: | ---: | | `/index.php` | 111.8 ns | 69.69 ns | 37.7% | 80 → 32 | 4 → 1 | | `/products/catalog/index.php` | 153.9 ns | 110.9 ns | 27.9% | 144 → 48 | 4 → 1 | | Long rooted path | 499.2 ns | 413.3 ns | 17.2% | 656 → 176 | 4 → 1 | These are helper-level results, not whole-request or RSS measurements. Relative paths had a 2.4% time reduction with unchanged allocations; traversal paths had an 11.0% reduction with Go B/op dropping from 96 to 72. The original benchmark cases were not rerun. Validation: - `gofmt` and `git diff --check` passed. - The exact helper and its repository path tests passed with `-race` in an isolated harness without PHP linkage. - A differential fuzz run against the original helper passed 657,119 executions. - The isolated tests cross-compiled for Windows; Windows runtime tests were not run locally. - Full FrankenPHP tests were not run: the local environment lacks the required PHP ZTS/shared-library and watcher setup. Adversarial benchmark cases use 4096 leading slashes and 512 rooted `a/../` segments. Separate exploratory measurements (Go 1.27.3, Apple M2, darwin/arm64; one 300ms sample per case): | Input | Before | After | Go B/op | allocs/op | | --- | ---: | ---: | ---: | ---: | | 4096 leading slashes | 3260 ns | 2782 ns | 9776 → 4912 | 4 → 3 | | 512 rooted `a/../` segments | 3533 ns | 3391 ns | 5424 → 2736 | 4 → 3 | These exploratory measurements do not establish statistical significance or an improvement on every input. Allocation counts and bytes do not increase in these cases. The branch also restores the `os` import needed by `os.ErrDeadlineExceeded`, which was lost when main was merged and caused the CI compilation failure.
1 parent 6dd16e2 commit d82fc4f

2 files changed

Lines changed: 4 additions & 1 deletion

File tree

‎cgi.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,7 @@ func sanitizedPathJoin(root, reqPath string) string {
345345

346346
// Clean with "path", not "filepath": on Windows filepath.Clean keeps the
347347
// ".." behind a leading "//" (UNC prefix), letting reqPath escape root.
348-
relPath := path.Clean("/" + reqPath)[1:]
348+
relPath := path.Clean(ensureLeadingSlash(reqPath))[1:]
349349
// A cleaned rooted path is always local on POSIX: only Windows needs the check
350350
if runtime.GOOS == "windows" && relPath != "" && !filepath.IsLocal(relPath) {
351351
// path is unsafe (see https://github.com/golang/go/issues/56336#issuecomment-1416214885)

‎cgi_test.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,9 @@ func TestSanitizedPathJoin(t *testing.T) {
5050
expected string
5151
}{
5252
{"leading slash", "/index.php", filepath.Join(root, "index.php")},
53+
{"multiple leading slashes", "///index.php", filepath.Join(root, "index.php")},
54+
{"multiple root slashes", "//", filepath.Clean(root) + separator},
55+
{"rooted nested traversal", "//foo/../../bar/", filepath.Join(root, "bar") + separator},
5356
{"no leading slash", "index.php", filepath.Join(root, "index.php")},
5457
{"nested", "/foo/bar/index.php", filepath.Join(root, "foo", "bar", "index.php")},
5558
{"empty req path returns root", "", filepath.Clean(root)},

0 commit comments

Comments
 (0)