fix: checkDepth should throw error if depth <= 0 #1989
Conversation
Depth can go below 0
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/invoke/utils.go (1)
9-9: Update function documentation to reflect new behavior.The current comment doesn't reflect that the function now returns an error for both zero and negative depth values.
-// checkDepth - a helper function that returns an error if the depth in a PermissionCheckRequest is zero. +// checkDepth - a helper function that returns an error if the depth in a PermissionCheckRequest is less than or equal to zero.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/invoke/utils.go(1 hunks)
🔇 Additional comments (3)
internal/invoke/utils.go (3)
12-12: LGTM! The fix correctly handles depth validation.The change from
== 0to<= 0properly addresses the race condition where concurrent goroutines could decrement the depth below zero. Usingatomic.LoadInt32ensures thread-safe access to the depth value.
13-13: LGTM! Error handling is appropriate.The error code
ERROR_CODE_DEPTH_NOT_ENOUGHclearly indicates the issue, and usingerrors.Newwith the error code's string representation maintains consistency with the error handling pattern.
12-12: Verify atomic operations on depth modifications.While this check uses
atomic.LoadInt32, we should ensure all depth modifications across the codebase also use atomic operations to maintain thread safety.✅ Verification successful
✓ Atomic operations are used consistently for depth modifications
All depth-related operations in the codebase properly use atomic operations:
atomic.LoadInt32for reading in utils.goatomic.AddInt32for modification in invoke.go🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct depth modifications without atomic operations rg -g '*.go' --type go -A 2 -B 2 'Depth\s*[-+]?=|Depth\s*[+-]{2}' # Search for proper atomic operations on depth rg -g '*.go' --type go -A 2 -B 2 'atomic\.(Add|Store|Load|Swap|CompareAndSwap).+Depth'Length of output: 840
This can be reproduced by making a complex request with a large depth. From my understanding, as the check trees grows the many simultaneous goroutines can decrement depth below 0 before the next
checkDepth, making this method run for unbounded time.Summary by CodeRabbit