DynamoRIO / DynamoRIO/dynamorio
Improve robustness in instrlist_disassemble
- Dominant language
- C
- Stars
- 3.2k
- Forks
- 629
- Avg merge
- 2d 15h
- Merged PRs (30d)
- 31
Description
I've got a problem where my bb instrumentation is creating a non-encodable instruction. I'm trying to diagnose that by doing instrlist_disassemble after the instrumentation, but that itself is crashing.
The relevant fragment is here in disassemble_shared.c:
```
nxt_pc = instr_encode_ignore_reachability(dcontext, instr, bytes);
ASSERT(nxt_pc != NULL);
```
In the default build configuration it seems that asserts are disabled and it then goes on to calculate a huge value of 'len' and eventually crashes. Perhaps for diagnostic code, asserts should be enabled by default at least in diagnostic printing routines, that are not going to be on the performance critical path.
But even if asserts were enabled, an assert fail probably isn't what you want here. What you more likely want is for instrlist_disassembly to tell you this instruction can't be encoded.
The fact that the root cause of this problem may be my user error in using the API is all the more reason to not have an assert fire deep in the DR code - surely that kind of assert is to find DR internal errors, things that by construction are supposed not to occur? So I'd suggest something like this:
```
@@ -1438,10 +1438,17 @@ instrlist_disassemble(dcontext_t *dcontext,
level = 4;
/* encode instr and then output as BINARY */
nxt_pc = instr_encode_ignore_reachability(dcontext, instr, bytes);
- ASSERT(nxt_pc != NULL);
- len = (int) (nxt_pc - bytes);
- addr = bytes;
- CLIENT_ASSERT(len < 64, "instrlist_disassemble: too-long instr");
+ if (nxt_pc != NULL) {
+ len = (int) (nxt_pc - bytes);
+ addr = bytes;
+ CLIENT_ASSERT(len < 64, "instrlist_disassemble: too-long instr");
+ } else {
+ print_file(outfile, " +%-4d!L4 @"PFX" ******************** ", offs, instr);
+ instr_disassemble(dcontext, instr, outfile);
+ print_file(outfile, "\n");
+ len = 0;
+ level = 0;
+ }
```
Contributor guide
Assessment
This issue has not been assessed yet.