AVM: Avoid panics while type checking bad immediates #5271
Conversation
jannotti
changed the title
| @@ -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) | |||
| 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..."}) |
There was a problem hiding this comment.
The ... usage in testMatch ~ testProg ~ assembler_test.go is mindblowing...
| @@ -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 | |||
There was a problem hiding this comment.
so it seems we previously allowed people to pass in -1 from getImm, and passed in scratchSpace, thus panic?
There was a problem hiding this comment.
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.
| 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"}) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Added tests for immediate bounds
Fixes #5217