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.

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

fix: race condition on StrID #1084

Merged
merged 2 commits into from
Jun 23, 2024
Merged

fix: race condition on StrID #1084

merged 2 commits into from
Jun 23, 2024

Conversation

M4tteoP
Copy link
Member

@M4tteoP M4tteoP commented Jun 22, 2024

WIP.
Implements StrID at rule parsing time in order to fix race conditions introduced by the lazy pattern. It basically creates the variable alongside the ID variable creation.
I'm still unsure about the StrRuleID_ name. The goal is to explain that it is not always just the string of ID_, but it is the "displayed" ID, so for chained rules, the displayed id is the parent id even if the inner rule (being an inner rule) has the 0 value ID.

Fixes #1083

Copy link

codecov bot commented Jun 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.06%. Comparing base (4ff1f76) to head (212eca5).
Report is 53 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1084      +/-   ##
==========================================
+ Coverage   82.72%   83.06%   +0.34%     
==========================================
  Files         162      164       +2     
  Lines        9080     7671    -1409     
==========================================
- Hits         7511     6372    -1139     
+ Misses       1319     1044     -275     
- Partials      250      255       +5     
Flag Coverage Δ
default 83.06% <100.00%> (+5.22%) ⬆️
examples 83.06% <100.00%> (+56.63%) ⬆️
ftw 83.06% <100.00%> (+35.69%) ⬆️
ftw-multiphase 83.06% <100.00%> (+33.52%) ⬆️
tinygo 83.06% <100.00%> (+7.66%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jcchavezs
Copy link
Member

Since this is mainly used in logs maybe call IDForLog_?

@M4tteoP
Copy link
Member Author

M4tteoP commented Jun 22, 2024

Confirming that the added test can reproduce the reported race condition:

▶ go test -timeout 60s -tags sqlite -run ^TestHttpServerConcurrent$ github.com/corazawaf/coraza/v3/examples/http-server -race
[...]
==================
WARNING: DATA RACE
Read at 0x00c0001223a8 by goroutine 88:
  github.com/corazawaf/coraza/v3/internal/corazarules.(*RuleMetadata).StrID()
      /Coraza/internal/corazarules/rule.go:89 +0x34
2024/06/22 14:52:35 [DEBUG] Evaluating phase tx_id="SynVkYwHnVfRyRdXEkR" phase=5
  github.com/corazawaf/coraza/v3/internal/corazawaf.(*Rule).doEvaluate()
      /Coraza/internal/corazawaf/rule.go:197 +0x120
2024/06/22 14:52:35 [DEBUG] Finished phase tx_id="SynVkYwHnVfRyRdXEkR" phase=5
  github.com/corazawaf/coraza/v3/internal/corazawaf.(*Rule).Evaluate()
      /Coraza/internal/corazawaf/rule.go:179 +0x364
  github.com/corazawaf/coraza/v3/internal/corazawaf.(*RuleGroup).Eval()
      /Coraza/internal/corazawaf/rulegroup.go:219 +0x670
  github.com/corazawaf/coraza/v3/internal/corazawaf.(*Transaction).ProcessRequestHeaders()
      /Coraza/internal/corazawaf/transaction.go:814 +0xf0
  github.com/corazawaf/coraza/v3/http.processRequest()
      /Coraza/http/middleware.go:63 +0x330
2024/06/22 14:52:35 [DEBUG] Transaction marked for audit logging tx_id="SynVkYwHnVfRyRdXEkR"
  github.com/corazawaf/coraza/v3/http.WrapHandler.func3()
      /Coraza/http/middleware.go:155 +0xb8
  net/http.HandlerFunc.ServeHTTP()
      /go1.20.7/src/net/http/server.go:2122 +0x48
  net/http.serverHandler.ServeHTTP()
      /go1.20.7/src/net/http/server.go:2936 +0x548
  net/http.(*conn).serve()
      /go1.20.7/src/net/http/server.go:1995 +0x8d8
  net/http.(*Server).Serve.func3()
      /go1.20.7/src/net/http/server.go:3089 +0x4c

Previous write at 0x00c0001223a8 by goroutine 73:
  github.com/corazawaf/coraza/v3/internal/corazarules.(*RuleMetadata).StrID()
      /Coraza/internal/corazarules/rule.go:94 +0x84
  github.com/corazawaf/coraza/v3/internal/corazawaf.(*Rule).doEvaluate()
      /Coraza/internal/corazawaf/rule.go:197 +0x120
  github.com/corazawaf/coraza/v3/internal/corazawaf.(*Rule).Evaluate()
      /Coraza/internal/corazawaf/rule.go:179 +0x364
  github.com/corazawaf/coraza/v3/internal/corazawaf.(*RuleGroup).Eval()
      /Coraza/internal/corazawaf/rulegroup.go:219 +0x670
  github.com/corazawaf/coraza/v3/internal/corazawaf.(*Transaction).ProcessRequestHeaders()
      /Coraza/internal/corazawaf/transaction.go:814 +0xf0
  github.com/corazawaf/coraza/v3/http.processRequest()
      /Coraza/http/middleware.go:63 +0x330
  github.com/corazawaf/coraza/v3/http.WrapHandler.func3()
      /Coraza/http/middleware.go:155 +0xb8
  net/http.HandlerFunc.ServeHTTP()
      /go1.20.7/src/net/http/server.go:2122 +0x48
  net/http.serverHandler.ServeHTTP()
      /go1.20.7/src/net/http/server.go:2936 +0x548
  net/http.(*conn).serve()
      /go1.20.7/src/net/http/server.go:1995 +0x8d8
  net/http.(*Server).Serve.func3()
      /go1.20.7/src/net/http/server.go:3089 +0x4c

@M4tteoP M4tteoP marked this pull request as ready for review June 22, 2024 14:18
@M4tteoP M4tteoP requested a review from a team as a code owner June 22, 2024 14:18
@M4tteoP M4tteoP merged commit 060b8ff into corazawaf:main Jun 23, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Race condition in RuleMetadata.StrID()
2 participants