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

AVM: Avoid panics while type checking bad immediates #5271

Merged
merged 1 commit into from
Apr 10, 2023
Merged

AVM: Avoid panics while type checking bad immediates #5271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 24 additions & 14 deletions data/transactions/logic/assembler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1158,8 +1158,9 @@ func asmDefault(ops *OpStream, spec *OpSpec, args []string) error {
return nil
}

// getImm interprets the arg at index argIndex as an immediate
func getImm(args []string, argIndex int) (int, bool) {
// getImm interprets the arg at index argIndex as an immediate that must be
// between -128 and 127 (if signed=true) or between 0 and 255 (if signed=false)
func getImm(args []string, argIndex int, signed bool) (int, bool) {
if len(args) <= argIndex {
return 0, false
}
Expand All @@ -1169,6 +1170,15 @@ func getImm(args []string, argIndex int) (int, bool) {
if err != nil {
return 0, false
}
if signed {
if n < -128 || n > 127 {
return 0, false
}
} else {
if n < 0 || n > 255 {
return 0, false
}
}
return int(n), true
}

Expand All @@ -1193,7 +1203,7 @@ func typeSwap(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes, err
}

func typeDig(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes, error) {
n, ok := getImm(args, 0)
n, ok := getImm(args, 0, false)
if !ok {
return nil, nil, nil
}
Expand All @@ -1210,7 +1220,7 @@ func typeDig(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes, erro
}

func typeBury(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes, error) {
n, ok := getImm(args, 0)
n, ok := getImm(args, 0, false)
if !ok {
return nil, nil, nil
}
Expand Down Expand Up @@ -1242,7 +1252,7 @@ func typeBury(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes, err
}

func typeFrameDig(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes, error) {
n, ok := getImm(args, 0)
n, ok := getImm(args, 0, true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

if !ok {
return nil, nil, nil
}
Expand All @@ -1263,7 +1273,7 @@ func typeFrameDig(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes,
}

func typeFrameBury(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes, error) {
n, ok := getImm(args, 0)
n, ok := getImm(args, 0, true)
if !ok {
return nil, nil, nil
}
Expand Down Expand Up @@ -1351,7 +1361,7 @@ func typeSetBit(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes, e
}

func typeCover(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes, error) {
n, ok := getImm(args, 0)
n, ok := getImm(args, 0, false)
if !ok {
return nil, nil, nil
}
Expand All @@ -1373,7 +1383,7 @@ func typeCover(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes, er
}

func typeUncover(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes, error) {
n, ok := getImm(args, 0)
n, ok := getImm(args, 0, false)
if !ok {
return nil, nil, nil
}
Expand Down Expand Up @@ -1402,7 +1412,7 @@ func typeTxField(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes,
}

func typeStore(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes, error) {
scratchIndex, ok := getImm(args, 0)
scratchIndex, ok := getImm(args, 0, false)
if !ok {
return nil, nil, nil
}
Expand All @@ -1428,16 +1438,16 @@ func typeStores(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes, e
}

func typeLoad(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes, error) {
scratchIndex, ok := getImm(args, 0)
scratchIndex, ok := getImm(args, 0, false)
if !ok {
return nil, nil, nil
}
return nil, StackTypes{pgm.scratchSpace[scratchIndex]}, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so it seems we previously allowed people to pass in -1 from getImm, and passed in scratchSpace, thus panic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I often forget how fragile the type* methods are. A lot of checking happens later, so I forget that these need good bounds checking on their own. Someday, I'd like to refactor so they are a bit safer/easier to write.

}

func typeProto(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes, error) {
a, aok := getImm(args, 0)
_, rok := getImm(args, 1)
a, aok := getImm(args, 0, false)
_, rok := getImm(args, 1, false)
if !aok || !rok {
return nil, nil, nil
}
Expand All @@ -1462,15 +1472,15 @@ func typeLoads(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes, er
}

func typePopN(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes, error) {
n, ok := getImm(args, 0)
n, ok := getImm(args, 0, false)
if !ok {
return nil, nil, nil
}
return anyTypes(n), nil, nil
}

func typeDupN(pgm *ProgramKnowledge, args []string) (StackTypes, StackTypes, error) {
n, ok := getImm(args, 0)
n, ok := getImm(args, 0, false)
if !ok {
return nil, nil, nil
}
Expand Down
28 changes: 28 additions & 0 deletions data/transactions/logic/assembler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3267,6 +3267,34 @@ add:
require.EqualError(t, err, "0: invalid syntax: not#pragma")
}

func TestAssembleImmediateRanges(t *testing.T) {
partitiontest.PartitionTest(t)
t.Parallel()

/* Perhaps all of these "unable to parse" errors could be improved to
discuss limits rather than bailout when the immediate is, in fact, an
integer. */

testProg(t, "int 1; store 0;", AssemblerMaxVersion)
testProg(t, "load 255;", AssemblerMaxVersion)

testProg(t, "int 1; store -1000;", AssemblerMaxVersion,
Expect{1, "store unable to parse..."})
testProg(t, "load -100;", AssemblerMaxVersion,
Expect{1, "load unable to parse..."})
testProg(t, "int 1; store 256;", AssemblerMaxVersion,
Expect{1, "store i beyond 255: 256"})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a gap in thinking through the error message: so failing to get type info w.r.t stack won't stop you from assembling, but what really stops you is in asmDefault where you call byteImm or int8Imm.

Does that sounds right about code path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, because the type* functions bailout and usually return nil, nil, nil when they are confused by the immediates or the stack size. They know that the real assembly routine will be angry, so they don't need to also complain.


testProg(t, "frame_dig -1;", AssemblerMaxVersion)
testProg(t, "frame_dig 127;", AssemblerMaxVersion)
testProg(t, "int 1; frame_bury -128;", AssemblerMaxVersion)

testProg(t, "frame_dig 128;", AssemblerMaxVersion,
Expect{1, "frame_dig unable to parse..."})
testProg(t, "int 1; frame_bury -129;", AssemblerMaxVersion,
Expect{1, "frame_bury unable to parse..."})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ... usage in testMatch ~ testProg ~ assembler_test.go is mindblowing...

}

func TestAssembleMatch(t *testing.T) {
partitiontest.PartitionTest(t)
t.Parallel()
Expand Down