Closed tmfink closed 5 years ago
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?
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 :-)
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():
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?
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).
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:
assert(...);
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
handled with #1489
The Problem
We can see that the M680X includes calls to
fprintf(stderr, "..")
:This would not work in code such as kernel code that do not support
Potential solutions
fprintf
linescs_err
variant; add handling incs_strerror()
csh
with something likecs_set_log_callback
; definecs_log_t