[M680X] errors logged to stdout, which is not suitable for kernel code

[tmfink]

The Problem

We can see that the M680X includes calls to fprintf(stderr, ".."):

$ grep -rn fprintf arch/ *.c *.h windows*
Binary file arch/M680X/M680XDisassembler.o matches
arch/M680X/M680XInstPrinter.c:339:              fprintf(stderr, "Internal error: Size mismatch in enum "
arch/M680X/M680XInstPrinter.c:346:              fprintf(stderr, "Internal error: Size mismatch in enum "
arch/M680X/M680XInstPrinter.c:353:              fprintf(stderr, "Internal error: Size mismatch in enum "
arch/M680X/M680XDisassembler.c:935:                     fprintf(stderr, "Internal error: Unexpected instruction "
arch/M680X/M680XDisassembler.c:1117:            fprintf(stderr, "Internal error: Unexpected operand0 register "
arch/M680X/M680XDisassembler.c:1563:            fprintf(stderr, "Internal error: Unexpected immediate byte "
arch/M680X/M680XDisassembler.c:1769:            fprintf(stderr, "Internal error: Unexpected post byte "
arch/M680X/M680XDisassembler.c:2148:            fprintf(stderr, "M680X_CPU_TYPE_%s is not suppported\n",
arch/M680X/M680XDisassembler.c:2227:            fprintf(stderr, "Internal error: Size mismatch in enum "
arch/M680X/M680XDisassembler.c:2234:            fprintf(stderr, "Internal error: Size mismatch in enum "
arch/M680X/M680XDisassembler.c:2241:            fprintf(stderr, "Internal error: Size mismatch in enum "
arch/M680X/M680XDisassembler.c:2248:            fprintf(stderr, "Internal error: Size mismatch in enum "
arch/M680X/M680XDisassembler.c:2255:            fprintf(stderr, "Internal error: Size mismatch in enum "
arch/M680X/M680XDisassembler.c:2262:            fprintf(stderr, "Internal error: Size mismatch in enum "
arch/M680X/M680XDisassembler.c:2269:            fprintf(stderr, "Internal error: Size mismatch in enum "
arch/M680X/M680XDisassembler.c:2276:            fprintf(stderr, "Internal error: Size mismatch in enum "
arch/M680X/M680XDisassembler.c:2283:            fprintf(stderr, "Internal error: Size mismatch in enum "
arch/M680X/M680XDisassembler.c:2290:            fprintf(stderr, "Internal error: Size mismatch in enum "
arch/M680X/M680XDisassembler.c:2297:            fprintf(stderr, "Internal error: Size mismatch in enum "
arch/M680X/M680XDisassembler.c:2304:            fprintf(stderr, "Internal error: Size mismatch in enum "

This would not work in code such as kernel code that do not support

$ nm libcapstone.so.5 | grep ' U' | awk '{ print $2 }' | sed 's/@@.*//' | sort -u
abort
__assert_fail
calloc
__fprintf_chk
free
malloc
memcpy
memmove
realloc
__sprintf_chk
__stack_chk_fail
stderr
strchr
strcmp
strcpy
__strcpy_chk
strlen
strncmp
strncpy
strrchr
strstr
strtol
vsnprintf

Potential solutions

1. Remove fprintf lines
   • simplest option
2. Add cs_err variant; add handling in cs_strerror()
3. Replace with other logging alternative
   • Allow user to define a logging callback in csh with something like cs_set_log_callback; define cs_log_t

[aquynh]

yes, i think a framework should not print out error, but propagate error code back to the higher level API.

@aladur what do you think?

[aladur]

1. As a fast workaround arch M680X can be disabled from being build by
   undefining CAPSTONE_HAS_M680X. I think that there will be no need to
   disassemble M680X code in a modern kernel :-)

2. The fprintf is rather a quick hack instead of an implementation.
   There are two types of errors:
   a) To avoid any confusion: The described error conditions can not occur
   when parsing illegal instructions. They only can occur if a maintainer
   (in the future) would make inappropriate changes to the code base.
   b) Errors are already handled in an appropriate way. The fprintf is just
   a reminder.

My proposed solution:

a) To be replaced by assert():
- All "Internal error: Size mismatch in enum" (could even be static_assert)
- "Internal error: Unexpected operand0 register"
- "Internal error: Unexpected immediate byte size"
b) To be completely removed:
- "Internal error: Unexpected instruction handler id"
- "Internal error: Unexpected post byte in loop instruction"
- "M680X_CPU_TYPE_%s is not suppported"

But when browsing other archs only RISCV uses assert(). All other archs (ARM,
AArch64, SystemZ, MIPS, X86, PowerPC, Sparc) consequently have commented
out assert(). From my point of view defective code should "fail early" which
easily can be done with assert().

@aquynh is the usage of assert() valid or is there a conscious desission to
comment it out?

[aquynh]

A quick answer: assert() makes the app exit, but a framework should never do that (but report back error, and let app decides what to do).

[aladur]

Yes, the app exit is a feature in this case. For example I check the static
size of an array with the last entry in an enum (the enum value is used as
index into the array).
If it fails this code should intentionally fail any gate check. It makes no
sense to let the user decide how to handle "internal errors" because he has
no choice. So my proposal is:

#if !defined(_KERNEL_MODE)
assert(...);
#endif

[tmfink]

#if !defined(_KERNEL_MODE)
assert(...);
#endif

Whether or not compiling for a kernel, it is not acceptable for library code to crash. The user of the library should decide whether to ignore, pass up, or abort.
The only exception is when memory corruption attacks occur (from which there is no safe way to proceed).

The code already correctly returns errors after printing, for example:
https://github.com/aquynh/capstone/blob/a095f344ced56ec9b817762dda6d1eeff247ce14/arch/M680X/M680XDisassembler.c#L935-L938

[tmfink]

handled with #1489