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

Jump to bottom

Data race warning detected upon starting Caddy #44

Closed
girlbossceo opened this issue Feb 13, 2023 · 3 comments · Fixed by #45
Closed

Data race warning detected upon starting Caddy #44

girlbossceo opened this issue Feb 13, 2023 · 3 comments · Fixed by #45

Comments

@girlbossceo
Copy link

Hi! Our servers build Caddy with Go's data race detector and I've observed the following data race detections upon each server restart when any request comes in: https://gist.github.com/r3g-5z/19a4710c2a061c3248d27de4dde5ddae (logs are too long)

At the moment I'm setting up Coraza so my CRS is all default except for allowlisting the protocols HTTP/3 and HTTP/3.0 (setvar:'tx.allowed_http_versions=HTTP/1.0 HTTP/1.1 HTTP/2 HTTP/2.0 HTTP/3 HTTP/3.0').

This warning only occurs once each restart, but it is consistent.

We're building Caddy from the main branch instead of the latest tag, and running and building with Go 1.20 (go version go1.20 linux/amd64).
@jcchavezs
Copy link
Member

Interesting. I think the main issue here is that the action is mutating the rule which sounds odd to me as rules are persistent across the lifecycle of the WAF and we are mutating rules that contain nolog on transaction basis. Let me try to remove that. cc @anuraaga (as corazawaf/coraza@fce26f2#diff-37cfb60455a57c6ad1051cd26e79e27f4bf533bd2cf7d09bac700c0974a73cf2R23) and @jptosso

jcchavezs added a commit to corazawaf/coraza that referenced this issue Feb 13, 2023
@jcchavezs
chore: avoids mutating the rule during evaluation.
1e7e567 
Initialization of actions happen once in the WASM and mutate the rules, however evaluation are executed per transaction and mutating the rule shouldn't happen as it leads to race conditions and unexpected behaviours. See corazawaf/coraza-caddy#44.
@jcchavezs jcchavezs mentioned this issue Feb 13, 2023
Merged
jcchavezs added a commit to corazawaf/coraza that referenced this issue Feb 13, 2023
@jcchavezs
chore: avoids mutating the rule during evaluation. (#645)
20a1ae3 
Initialization of actions happen once in the WASM and mutate the rules, however evaluation are executed per transaction and mutating the rule shouldn't happen as it leads to race conditions and unexpected behaviours. See corazawaf/coraza-caddy#44.
@jptosso jptosso mentioned this issue Feb 13, 2023
Merged
@jptosso jptosso linked a pull request Feb 13, 2023 that will close this issue
chore: update coraza version #45
Merged
@jptosso
Copy link
Member

jptosso commented Feb 13, 2023

Thank you for your report, fixed in #45 which implements @jcchavezs fix

@girlbossceo
Copy link
Author

Thanks! Will build and try again later

jcchavezs added a commit to corazawaf/coraza that referenced this issue Feb 17, 2023
@jcchavezs
Improves actions code (#646)
8804923 
* chore: avoids mutating the rule during evaluation.

Initialization of actions happen once in the WASM and mutate the rules, however evaluation are executed per transaction and mutating the rule shouldn't happen as it leads to race conditions and unexpected behaviours. See corazawaf/coraza-caddy#44.

* chore: uses blank identifier to better analyse the API and mutations.

* tests: improves coverage.

* tests: adds more tests.

* wip

* chore: group errors.

* tests: adds more coverage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chore: update coraza version corazawaf/coraza-caddy
3 participants
@jptosso @jcchavezs @girlbossceo