Conversation
Changes SummaryThis PR updates Caddy to use Go's math/rand/v2 package (introduced in Go 1.22) instead of the legacy math/rand package. The changes include replacing deprecated methods like Intn() with IntN(), Int63() with Int64(), and updating random source initialization with ChaCha8-based generators. Type: refactoring Components Affected: HTTP authentication (basicauth), HTTP error handling, File server (static files), Reverse proxy (selection policies, streaming, upstreams), ACME server, FastCGI client, Integration tests Files Changed
Architecture Impact
Risk Areas: Random number generation correctness - IntN() has different semantics than Intn(), Modulo operation replacement - Converting (weakrand.Int() % count) == 0 to weakrand.IntN(count) == 0 changes the random distribution slightly but achieves equivalent logic, ChaCha8 initialization - listener_test.go uses rand.NewChaCha8([32]byte{}) which initializes with a zero seed for deterministic testing Suggestions
Full review in progress... | Powered by diffray |
| @@ -244,7 +244,7 @@ func (c *Cache) makeRoom() { | |||
| // strategy; generating random numbers is cheap and | |||
There was a problem hiding this comment.
🔴 CRITICAL - Off-by-one error in cache eviction loop condition
Agent: go
Category: bug
Description:
Loop condition 'deleted <= numToDelete' causes deletion of numToDelete+1 items instead of numToDelete. With numToDelete=10, the loop runs for deleted values 0 through 10 inclusive (11 iterations), deleting 11 items instead of 10.
Suggestion:
Change '<=' to '<' on line 240: 'for deleted := 0; deleted < numToDelete; deleted++'
Confidence: 95%
Rule: bug_slice_bounds_go
Review ID: 9bac5f9f-38e7-46a1-add2-7c064de630b1
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| rnd := weakrand.IntN(len(c.cache)) | ||
| i := 0 | ||
| for key := range c.cache { | ||
| if i == rnd { |
There was a problem hiding this comment.
🟠 HIGH - Inefficient O(n*m) cache eviction with repeated map traversals
Agent: go
Category: performance
Description:
The makeRoom() function performs numToDelete iterations, each generating a random index and iterating through the entire map to find and delete that element. This creates O(n*m) complexity where n is cache size and m is items to delete.
Suggestion:
Optimize by collecting all cache keys into a slice once, shuffling, and deleting the first numToDelete keys in a single pass for O(n) complexity.
Confidence: 85%
Rule: go_use_defer_to_release_resources
Review ID: 9bac5f9f-38e7-46a1-add2-7c064de630b1
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| // 1 more than an MB | ||
| body := make([]byte, uploadSize) | ||
| rand.New(rand.NewSource(0)).Read(body) | ||
| rand.NewChaCha8([32]byte{}).Read(body) |
There was a problem hiding this comment.
🟡 MEDIUM - Zero-initialized random seed produces deterministic test data
Agent: go
Category: quality
Description:
Using 'rand.NewChaCha8([32]byte{})' with a zero seed produces identical test data on every run. While this may be intentional for reproducibility, the test name suggests testing with varied data.
Suggestion:
If determinism is intended, add a comment explaining the choice. If varied data is desired, use a non-zero or time-based seed.
Confidence: 65%
Rule: qual_misleading_names_go
Review ID: 9bac5f9f-38e7-46a1-add2-7c064de630b1
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
Review Summary
Validated 9 issues: 5 kept (1 critical off-by-one bug, 2 performance/quality concerns, 2 security/quality), 4 filtered (2 incorrect API claims, 1 false positive on guarded code, 1 low-value exported field concern) Issues Found: 5💬 See 3 individual line comment(s) for details. 📋 Full issue list (click to expand)🔴 CRITICAL - Off-by-one error in cache eviction loop conditionAgent: go Category: bug File: Description: Loop condition 'deleted <= numToDelete' causes deletion of numToDelete+1 items instead of numToDelete. With numToDelete=10, the loop runs for deleted values 0 through 10 inclusive (11 iterations), deleting 11 items instead of 10. Suggestion: Change '<=' to '<' on line 240: 'for deleted := 0; deleted < numToDelete; deleted++' Confidence: 95% Rule: 🟠 HIGH - Inefficient O(n*m) cache eviction with repeated map traversalsAgent: go Category: performance File: Description: The makeRoom() function performs numToDelete iterations, each generating a random index and iterating through the entire map to find and delete that element. This creates O(n*m) complexity where n is cache size and m is items to delete. Suggestion: Optimize by collecting all cache keys into a slice once, shuffling, and deleting the first numToDelete keys in a single pass for O(n) complexity. Confidence: 85% Rule: 🟡 MEDIUM - Zero-initialized random seed produces deterministic test dataAgent: go Category: quality File: Description: Using 'rand.NewChaCha8([32]byte{})' with a zero seed produces identical test data on every run. While this may be intentional for reproducibility, the test name suggests testing with varied data. Suggestion: If determinism is intended, add a comment explaining the choice. If varied data is desired, use a non-zero or time-based seed. Confidence: 65% Rule: 🟡 MEDIUM - Slice aliasing: defaultIndexNames assigned without copyAgent: security Category: security File: Description: The IndexNames field is assigned the module-level defaultIndexNames slice directly without a defensive copy. If IndexNames is modified later, it will corrupt the shared default. Suggestion: Create a defensive copy: fsrv.IndexNames = append([]string(nil), defaultIndexNames...) Confidence: 75% Rule: 🟡 MEDIUM - Global variable test isolation violation - globalt not restoredAgent: testing Category: quality File: Description: The global variable globalt (*testing.T) is written to in DisabledTest without being restored via defer, violating test isolation principles. Suggestion: Remove the global globalt variable and pass *testing.T as a parameter through the function call chain to maintain test isolation. Confidence: 80% Rule: ℹ️ 2 issue(s) outside PR diff (click to expand)
🟡 MEDIUM - Slice aliasing: defaultIndexNames assigned without copyAgent: security Category: security File: Description: The IndexNames field is assigned the module-level defaultIndexNames slice directly without a defensive copy. If IndexNames is modified later, it will corrupt the shared default. Suggestion: Create a defensive copy: fsrv.IndexNames = append([]string(nil), defaultIndexNames...) Confidence: 75% Rule: 🟡 MEDIUM - Global variable test isolation violation - globalt not restoredAgent: testing Category: quality File: Description: The global variable globalt (*testing.T) is written to in DisabledTest without being restored via defer, violating test isolation principles. Suggestion: Remove the global globalt variable and pass *testing.T as a parameter through the function call chain to maintain test isolation. Confidence: 80% Rule: Review ID: |
4 participants
Assistance Disclosure
No AI is used.
Some of the modulo calculations have been changed to
IntNfor simplicity.