RISC-V 32: Crash when trying to display disassembly code

[ghostiam]

Environment information

• Operating System: MacOS 14.5 (ARM-64 - M1)
• Cutter version: 2.3.4-stable-209c26b
• Obtained from:
  • Built from source
  • Downloaded release from Cutter website or GitHub
  • Distribution repository (brew)
• File format:
  ELF (RISC-V 32)

Describe the bug

Crash when trying to display disassembly code.

MacOS report

-------------------------------------
Translated Report (Full Report Below)
-------------------------------------

Process:               cutter [75863]
Path:                  /Applications/Cutter.app/Contents/MacOS/cutter
Identifier:            re.rizin.cutter
Version:               2.3.4-stable-209c26b (2.3.4-stable-209c26b)
Code Type:             ARM-64 (Native)
Parent Process:        launchd [1]
User ID:               501

Date/Time:             2024-07-24 23:23:32.0016 +0400
OS Version:            macOS 14.5 (23F79)
Report Version:        12
Anonymous UUID:        5BC71BD9-F14A-2CB9-7C3A-A5EA18D4D253

Sleep/Wake UUID:       FF657A00-3D1F-4900-8568-BBFC9479AF65

Time Awake Since Boot: 1800000 seconds
Time Since Wake:       114638 seconds

System Integrity Protection: enabled

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000000
Exception Codes:       0x0000000000000001, 0x0000000000000000

Termination Reason:    Namespace SIGNAL, Code 11 Segmentation fault: 11
Terminating Process:   exc handler [75863]

VM Region Info: 0 is not in any region.  Bytes before following region: 4307992576
      REGION TYPE                    START - END         [ VSIZE] PRT/MAX SHRMOD  REGION DETAIL
      UNUSED SPACE AT START
--->  
      __TEXT                      100c6c000-100f84000    [ 3168K] r-x/r-x SM=COW  /Applications/Cutter.app/Contents/MacOS/cutter

Thread 0 Crashed::  Dispatch queue: com.apple.main-thread
0   libsystem_platform.dylib      	       0x1848393c8 _platform_strstr + 32
1   librz_core.0.7.dylib          	       0x101fdd6cc ds_print_ptr + 2636
2   librz_core.0.7.dylib          	       0x101fd8a84 rz_core_print_disasm + 14536
3   cutter                        	       0x100ccf084 0x100c6c000 + 405636
4   cutter                        	       0x100cce59c 0x100c6c000 + 402844
5   cutter                        	       0x100cce1c0 0x100c6c000 + 401856
6   QtCore                        	       0x103d300ec 0x103b28000 + 2130156
7   cutter                        	       0x100c7bab8 CutterSeekable::seekableSeekChanged(unsigned long long, CutterCore::SeekHistoryType) + 72
8   QtCore                        	       0x103d300ec 0x103b28000 + 2130156
9   cutter                        	       0x100c7eaf0 CutterCore::seekChanged(unsigned long long, CutterCore::SeekHistoryType) + 72
10  cutter                        	       0x100c9abd8 CutterCore::seek(unsigned long long) + 140
11  cutter                        	       0x100c99ebc CutterCore::seekAndShow(unsigned long long) + 20
12  QtCore                        	       0x103d300ec 0x103b28000 + 2130156
13  QtWidgets                     	       0x10271d99c QAbstractItemView::activated(QModelIndex const&) + 52
14  QtWidgets                     	       0x10278a16c QTreeView::mouseDoubleClickEvent(QMouseEvent*) + 868
15  QtWidgets                     	       0x10250ea1c QWidget::event(QEvent*) + 128
16  QtWidgets                     	       0x1025a31e0 QFrame::event(QEvent*) + 56
17  QtWidgets                     	       0x10271c450 QAbstractItemView::viewportEvent(QEvent*) + 1124
18  QtWidgets                     	       0x102786cec QTreeView::viewportEvent(QEvent*) + 500
19  QtCore                        	       0x103d0015c QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) + 264
20  QtWidgets                     	       0x1024d8d58 QApplicationPrivate::notify_helper(QObject*, QEvent*) + 260
21  QtWidgets                     	       0x1024db6a4 QApplication::notify(QObject*, QEvent*) + 6072
22  QtCore                        	       0x103cffe44 QCoreApplication::notifyInternal2(QObject*, QEvent*) + 208
23  QtWidgets                     	       0x1024d96dc QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer<QWidget>&, bool, bool) + 968
24  QtWidgets                     	       0x10252c250 0x1024c8000 + 410192
25  QtWidgets                     	       0x10252b280 0x1024c8000 + 406144
26  QtWidgets                     	       0x1024d8d78 QApplicationPrivate::notify_helper(QObject*, QEvent*) + 292
27  QtWidgets                     	       0x1024da110 QApplication::notify(QObject*, QEvent*) + 548
28  QtCore                        	       0x103cffe44 QCoreApplication::notifyInternal2(QObject*, QEvent*) + 208
29  QtGui                         	       0x10300b23c QGuiApplicationPrivate::processMouseEvent(QWindowSystemInterfacePrivate::MouseEvent*) + 4436
30  QtGui                         	       0x102ff0558 QWindowSystemInterface::sendWindowSystemEvents(QFlags<QEventLoop::ProcessEventsFlag>) + 248
31  libqcocoa.dylib               	       0x101292d58 0x10125c000 + 224600
32  CoreFoundation                	       0x1848ea4d8 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 28
33  CoreFoundation                	       0x1848ea46c __CFRunLoopDoSource0 + 176
34  CoreFoundation                	       0x1848ea1dc __CFRunLoopDoSources0 + 244
35  CoreFoundation                	       0x1848e8dc8 __CFRunLoopRun + 828
36  CoreFoundation                	       0x1848e8434 CFRunLoopRunSpecific + 608
37  HIToolbox                     	       0x18f08c19c RunCurrentEventLoopInMode + 292
38  HIToolbox                     	       0x18f08be2c ReceiveNextEventCommon + 220
39  HIToolbox                     	       0x18f08bd30 _BlockUntilNextEventMatchingListInModeWithFilter + 76
40  AppKit                        	       0x188147d68 _DPSNextEvent + 660
41  AppKit                        	       0x18893d808 -[NSApplication(NSEventRouting) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 700
42  AppKit                        	       0x18813b09c -[NSApplication run] + 476
43  libqcocoa.dylib               	       0x101291b6c 0x10125c000 + 220012
44  QtCore                        	       0x103cfbf1c QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) + 524
45  QtCore                        	       0x103d00470 QCoreApplication::exec() + 132
46  cutter                        	       0x100c947c0 0x100c6c000 + 165824
47  dyld                          	       0x1844820e0 start + 2360

To Reproduce

Steps to reproduce the behavior:

1. Open file: blink-crash.elf from blink.zip

2. Try open function blink.copy_data

3. <Crash>

Expected behavior

Show disassembly code.

Additional context

If I remove the copy_data function, then the crash does not occur(file blink-no-copy_data.elf in archive).

[ghostiam]

The crash occurs because the realname field is NULL.

rizin/librz/core/disasm.c

Line 4076 in 706a6bf

} else if (!flag_printed && (!ds->opstr || (!strstr(ds->opstr, f->name) && !strstr(ds->opstr, f->realname)))) {

I also have another file that also causes a crash, but in a different place, where the realname field is also to blame.

rizin/librz/core/canalysis.c

Line 5696 in e4958fc

p->realname = strdup(f->realname);

This quick fix helped (but I'm not sure it's correct):

diff --git a/librz/flag/flag.c b/librz/flag/flag.c
index 1baee1dff6..84367de61d 100644
--- a/librz/flag/flag.c
+++ b/librz/flag/flag.c
@@ -680,7 +680,7 @@ RZ_API void rz_flag_item_set_comment(RzFlagItem *item, const char *comment) {
 RZ_API void rz_flag_item_set_realname(RzFlagItem *item, const char *realname) {
        rz_return_if_fail(item);
        free_item_realname(item);
-       item->realname = RZ_STR_ISEMPTY(realname) ? NULL : strdup(realname);
+       item->realname = RZ_STR_ISEMPTY(realname) ? item->name : strdup(realname);
 }
 
 /* add/replace/remove the color of a flag item */

[XVilka]

1. Crash should be fixed regardless but
2. Please keep in mind that the RISC-V support in Rizin is outdated. We have a GSoC student @moste00 working on updating it first in Capstone and then in Rizin itself. See https://rizin.re/posts/gsoc-2024-announcement/#moste00-uplifting-risc-v-instructions-to-rzil for more details (also auto-sync progress tracker: Refactor and implement architectures capstone-engine/capstone#2015)

[XVilka]

@moste00 could you please help us to fix this crash before the release? It will help to delve into the Rizin architecture code as well.

[moste00]

@moste00 could you please help us to fix this crash before the release? It will help to delve into the Rizin architecture code as well.

yeah of course no problem, should I commit the quick fix by @ghostiam or investigate deeper why the name is NULL in the first place ?

[XVilka]

@moste00 could you please help us to fix this crash before the release? It will help to delve into the Rizin architecture code as well.

yeah of course no problem, should I commit the quick fix by @ghostiam or investigate deeper why the name is NULL in the first place ?

Quick fix is enough, since the RISC-V code will be completely rewritten after the Capstone update is finished.

[moste00]

@XVilka Hello, I investigated this crash a bit and here's what I think about it.

(1) This, almost certainly, has nothing to do with RISC-V. The crash-relevant code paths has no RISC-V logic in it, just Elf handling

(2) Similarly, the crash also has nothing to do with the "culprit" function copy_data. The problemetic offsets are in another function blink_delay, and when cutter opens the crash-inducing binary, clicking on any function except main will reproduce the crash

(3) The crash is an Elf issue:

• There are nameless symbols in the binary (completely normal, allowed by the Elf format)

• Those nameless symbols get assigned FlagItems (probably rizin's terminology for things that annotate Elf symbols), those FlagItems have name and realname members

• name members are always non-NULL because they get assigned to an auto-generated string derieved from the offset they annotate (e.g. loc_.0xf00) when creating the FlagItem instance

• realname members, however, are initially the same as the name, but later get assigned NULL as a consequence of their setter functions (quoted above by @ghostiam), which assigns to NULL if the passed value is NULL or an empty string (starts with '\0').

• This happens a lot, no less than 1000+ times.

• But for some reason, only 2 symbols in the crash-inducing binary actually do cause a crash, one at offset 0x112 and another at offset 0x2c8 (the first is an instruction in blink_delay function, the second is a weird part of the binary that Rizin displays annotated with ZigCompilerBuiltin or something like that. That's another piece of probably-not-relevant information: the binary is the output of a Zig compiler.)

• The crashing function is ds_print_ptr, whose documentation ostensibly says that it just converts numeric instruction arguments to hex strings, but whose actual implementation is a massive 300+ blob of code having 5 and 6 nested conditionals, among other confusing logic. The part that induces the crash is indeed the snippet pointed out by @ghostiam, a call to strstr on the NULL second argument realname (undefined behaviour according to the spec of strstr, segfault crash in practice)

• As previously said, it's a big mystery why only those 2 problematic offsets arrive at the crashing logic, and not the other hundreds of FlagItems with NULL realname members. I tried to investigate ds_print_ptr's flow but the function is too massive, so in all honesty I got stuck, all I know is that it is called a lot, but the crashing logic is only hit 2 times for some reason

• Deleting the function copy_data simply has the effect of re-arranging the binary so that those offsets don't have symbols, so don't get annotated with FlagItem instances, and the whole problem disappear

(4) The crash has several possible fixes, but none strike me as ideal

• We could do @ghostiam suggestion, ensuring that realname is never "more NULL" than name is. But this has the effect that it's no impossible to "delete" realname, which the documentation of the setter function says can be done. What if there are other use cases in which deleting a symbol is valid ?

• We could ensure the specific caller of the setter in the crash-relevant code paths never assigns NULL, which achieves the same thing as above but leaves other callers free to assign NULLs

• We could simply && ->realname before every use of realname in ds_print_ptr.

I personally like the second solution better, it solves the issue with minimum assumptions. If it's super important to know why is it the case that only 0x112 and 0x2c8 crash despite the thuosands of other nameless symbols, I can investigate more, but I didn't want to waste more time.

[XVilka]

@moste00 go ahead with the second one but please open a new issue, since it's only a temporary bandaid and the underlying issue is a big one and would need some refactoring.

[moste00]

@XVilka the PR fixing this is ready for review