Rule out sync bugs due to slice reuse (fixes #198) #199
Conversation
| } | ||
| if c.allowCredentials { | ||
| headers["Access-Control-Allow-Credentials"] = headerTrue | ||
| headers.Set("Access-Control-Allow-Credentials", "true") |
There was a problem hiding this comment.
To keep the allocation optimization and avoid triggering the concurrency check, we could create a new slice with the same backing array. Something like headerTrue[:1]
There was a problem hiding this comment.
But then handlers could still mutate the first element of the shared backing array through the new slice...
There was a problem hiding this comment.
They only append or set right? There should no slide[0] = val as far as I remember
There was a problem hiding this comment.
Sorry, I don't understand your last comment... 🤔
What I meant in my earlier comment is that, even if you write
headers["Access-Control-Allow-Credentials"] = headerTrue[:1]the tests that I posted in #198 keep failing, partly because the first element of headerTrue can still be mutated.
There was a problem hiding this comment.
The failing tests included in #198 show how. Here is a version of the mutating handler focused on ACAO:
func(w http.ResponseWriter, r *http.Request) {
acao := w.Header()["Access-Control-Allow-Credentials"] // aliases slices headerTrue, headerTrue[:1], etc.
if len(acao) > 0 {
acao[0] = "oops!"
fmt.Println(headerTrue[0]) // oops!
}
}There was a problem hiding this comment.
This is an unlikely use case. The caller would have to assume there is a first element they did not add and would know what it is.
There was a problem hiding this comment.
You may be right about the unlikelihood of such a use case, but the fact remains: a handler can corrupt a CORS middleware that wraps it and inadvertently introduce non-obvious synchronisation bugs (non-obvious because it would be natural to assume that the acao slice doesn't alias anything at package level of rs/cors). I think most users of your library would value the guarantee that no such bugs are possible more than saving a couple of allocations during middleware execution.
There was a problem hiding this comment.
@rs For completeness, here are some benchmark results ("old" and "new" correspond to master and my branch, respectively):
goos: darwin
goarch: arm64
pkg: github.com/rs/cors
cpu: Apple M4
│ old │ new │
│ sec/op │ sec/op vs base │
Without-10 3.279n ± 2% 3.265n ± 6% ~ (p=0.732 n=6)
Default-10 98.96n ± 2% 184.55n ± 14% +86.48% (p=0.002 n=6)
AllowedOrigin-10 142.0n ± 1% 160.5n ± 10% +13.10% (p=0.002 n=6)
Preflight-10 247.7n ± 3% 339.9n ± 11% +37.20% (p=0.002 n=6)
PreflightHeader-10 269.6n ± 2% 358.3n ± 3% +32.93% (p=0.002 n=6)
PreflightAdversarialACRH-10 501.1n ± 18% 536.6n ± 2% ~ (p=0.065 n=6)
Wildcard/match-10 6.625n ± 1% 6.612n ± 1% ~ (p=0.240 n=6)
Wildcard/too_short-10 0.9658n ± 22% 0.9652n ± 3% ~ (p=0.413 n=6)
geomean 42.10n 50.20n +19.26%
│ old │ new │
│ B/op │ B/op vs base │
Without-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=6) ¹
Default-10 16.00 ± 0% 48.00 ± 0% +200.00% (p=0.002 n=6)
AllowedOrigin-10 16.00 ± 0% 32.00 ± 0% +100.00% (p=0.002 n=6)
Preflight-10 16.00 ± 0% 48.00 ± 0% +200.00% (p=0.002 n=6)
PreflightHeader-10 16.00 ± 0% 48.00 ± 0% +200.00% (p=0.002 n=6)
PreflightAdversarialACRH-10 40.00 ± 0% 56.00 ± 0% +40.00% (p=0.002 n=6)
Wildcard/match-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=6) ¹
Wildcard/too_short-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=6) ¹
geomean ² +71.72% ²
¹ all samples are equal
² summaries must be >0 to compute geomean
│ old │ new │
│ allocs/op │ allocs/op vs base │
Without-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=6) ¹
Default-10 1.000 ± 0% 3.000 ± 0% +200.00% (p=0.002 n=6)
AllowedOrigin-10 1.000 ± 0% 2.000 ± 0% +100.00% (p=0.002 n=6)
Preflight-10 1.000 ± 0% 3.000 ± 0% +200.00% (p=0.002 n=6)
PreflightHeader-10 1.000 ± 0% 3.000 ± 0% +200.00% (p=0.002 n=6)
PreflightAdversarialACRH-10 2.000 ± 0% 3.000 ± 0% +50.00% (p=0.002 n=6)
Wildcard/match-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=6) ¹
Wildcard/too_short-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=6) ¹
geomean ² +73.21% ²
¹ all samples are equal
² summaries must be >0 to compute geomean
2 participants
No description provided.