Security/forbid halt #21
Conversation
WalkthroughThe pull request introduces significant changes to the handling of the Changes
Possibly related PRs
Suggested reviewers
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## main #21 +/- ##
=======================================
Coverage 97.86% 97.87%
=======================================
Files 24 24
Lines 7840 7844 +4
=======================================
+ Hits 7673 7677 +4
Misses 134 134
Partials 33 33
🚨 Try these New Features:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
engine/promise.go (2)
Line range hint
220-228: Remove unused PanicError typeThe
PanicErrortype appears to be unused after the recent changes topanicErrorfunction, which now returnsExceptioninstead.-// PanicError is an error thrown once panic occurs during the execution of a promise. -type PanicError struct { - OriginErr error -} - -func (p PanicError) Error() string { - return fmt.Sprintf("panic: %v", p.OriginErr) -}
168-170: Consider enhancing error messagesThe error messages could be more descriptive to help with debugging. Consider including additional context about where the panic occurred.
- return Exception{term: atomError.Apply(NewAtom("panic_error").Apply(NewAtom(r.Error())))} + return Exception{term: atomError.Apply(NewAtom("panic_error").Apply(NewAtom(fmt.Sprintf("promise execution: %v", r.Error()))))} - return Exception{term: atomError.Apply(NewAtom("panic_error").Apply(NewAtom(fmt.Sprintf("%v", r))))} + return Exception{term: atomError.Apply(NewAtom("panic_error").Apply(NewAtom(fmt.Sprintf("promise execution: %v", r))))}README.md (1)
45-45: Enhance documentation with implementation detailsThe documentation clearly states that
halt/0andhalt/1are forbidden. However, consider adding more context to help users understand and handle this restriction:- - `halt/0` and `halt/1` are forbidden and will throw an error. + - `halt/0` and `halt/1` are forbidden and will throw `error(panic_error(halt is not allowed))`. This restriction ensures safe execution within the blockchain environment by preventing uncontrolled termination of the Prolog VM.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between d446d5b and 04c24f957841806139a28bc36d0672397311f5b3.
📒 Files selected for processing (4)
README.md(1 hunks)engine/builtin.go(1 hunks)engine/builtin_test.go(3 hunks)engine/promise.go(1 hunks)
🔇 Additional comments (3)
engine/promise.go (2)
165-171: LGTM: Error handling improvements look good
The changes to panicError improve error handling consistency by wrapping all panic scenarios in the Exception type. This aligns well with the PR's objective of properly handling panics from the halt/1 restriction.
165-171: Verify error handling changes across the codebase
Let's ensure these changes don't break existing error handling and that all panic scenarios are properly handled.
✅ Verification successful
Let me gather more information about the error handling changes.
Changes to panic error handling are safe and properly implemented
The code changes are safe because:
- The
PanicErrortype is only used internally withinengine/promise.go - The panic recovery mechanism is properly maintained through
recover()function - The changes only modify the error term construction while preserving the error handling flow
- The error handling chain (through
promiseStack.recover) remains intact
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining uses of PanicError
echo "Checking for PanicError usage..."
rg "PanicError"
# Check for existing panic handling patterns
echo "Checking panic handling patterns..."
ast-grep --pattern 'recover() {
$$$
}'
# Look for potential error handling test cases
echo "Checking error handling tests..."
rg -l "test.*Error" --type go
Length of output: 543
Script:
#!/bin/bash
# Check the implementation and usage of PanicError
echo "Checking PanicError implementation..."
rg -A 5 "type PanicError struct" engine/promise.go
# Check recover() usage in promise.go
echo "Checking recover usage in promise.go..."
rg -B 5 -A 10 "recover()" engine/promise.go
# Check error handling tests
echo "Checking error handling test cases..."
cat engine/text_test.go | rg "test.*Error" -A 5
Length of output: 3514
engine/builtin_test.go (1)
73-77: Verify that panics with integer literals are properly handled
The test case for do_not_call_misc_error expects an exception with "panic_error" wrapping the string "42". Since the code panics with an integer literal 42, verify that this is correctly converted to a string in the error handling logic.
Run the following script to confirm that panics with an integer are appropriately handled:
✅ Verification successful
Let me gather more information about how panics are handled in the codebase.
Based on the results, I can now generate the final response as I found the relevant panic handling logic in the codebase.
Panics with integer literals are properly handled
The panic value 42 is correctly converted to a string in the error handling logic. This is confirmed by the implementation in engine/promise.go which uses fmt.Sprintf("%v", r) to convert any panic value (including integers) to a string when creating the panic_error atom. The test case correctly expects "42" as the string representation of the integer panic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Ensure panics with integer literals are correctly converted to errors.
# Search for panic calls with integer literals
rg 'panic\([0-9]+\)' engine/
# Check how integer panics are converted to strings in error handling
rg 'func recover' engine/promise.go -A 10
# Verify that the error message in the test matches the converted panic value
rg 'panic_error' engine/builtin_test.go -A 5
Length of output: 1441
Script:
#!/bin/bash
# Search for panic handling and recovery logic
ast-grep --pattern 'func $name($$$) {
$$$
if r := recover(); r != nil {
$$$
}
$$$
}'
# Look for any string conversion or formatting of panic values
rg 'fmt\.Sprint|fmt\.Sprintf|String\(\)|ToString' engine/
Length of output: 7300
ccamel
force-pushed
the
security/forbid-halt
branch
from
04c24f9
to
3a63e6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
engine/builtin_test.go (1)
5530-5534: Consider adding more test casesWhile the current test verifies the basic functionality, consider adding test cases for:
- Different integer values passed to halt/1
- Edge cases like MaxInt64/MinInt64
- Multiple consecutive calls to halt/1
Example test cases:
t.Run("edge cases", func(t *testing.T) { cases := []int64{math.MaxInt64, math.MinInt64, 0, 42} for _, n := range cases { ok, err := Delay(func(ctx context.Context) *Promise { return Halt(nil, Integer(n), Success, nil) }).Force(context.Background()) assert.EqualError(t, err, "error(panic_error(halt/1 is not allowed))") assert.False(t, ok) } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 04c24f957841806139a28bc36d0672397311f5b3 and 3a63e6e.
📒 Files selected for processing (3)
README.md(1 hunks)engine/builtin.go(1 hunks)engine/builtin_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
📓 Learnings (2)
engine/builtin.go (1)
Learnt from: ccamel
PR: axone-protocol/prolog#21
File: engine/builtin.go:1963-1965
Timestamp: 2024-11-24T08:43:47.451Z
Learning: In `engine/builtin.go`, the use of `panic` in the `osExit` function to handle `halt/1` is intentional and should not be altered to return an error instead.
engine/builtin_test.go (1)
<retrieved_learning>
Learnt from: ccamel
PR: #21
File: engine/builtin_test.go:40-45
Timestamp: 2024-11-24T08:47:15.195Z
Learning: In engine/builtin_test.go, the function do_not_call_misc_error intentionally panics with a non-string, non-error value (e.g., panic(42)) as part of the test. Do not suggest changing this code to panic with an error value.
</retrieved_learning>
🔇 Additional comments (2)
engine/builtin.go (1)
1963-1965: LGTM! Security improvement to prevent VM termination.
The implementation effectively forbids the use of halt/1 predicate by replacing the os.Exit call with a panic, which is crucial for maintaining the integrity of the blockchain's VM. The error message clearly indicates that the operation is not allowed.
engine/builtin_test.go (1)
5530-5534: LGTM! Test verifies the new halt/1 behavior
The test correctly verifies that halt/1 now returns an error instead of actually halting, with the proper error message format.
This PR enforces the disallowance of the
halt/1predicate, which now throws the errorerror(panic_error('halt/1 is not allowed'))when invoked.Previously, this predicate was unregistered in the axone blockchain’s
logicmodule (axone-protocol/axoned@293da10). However, as the VM is now designed to be safely embedded in a blockchain, this predicate must be explicitly controlled at the VM level to ensure robust security.