cebix / macemu

Basilisk II and SheepShaver Macintosh emulators
1.39k stars 289 forks source link

Cxmon's prompt overflows its buffer on 64-bit hosts #240

Closed CharlesHawkins closed 1 month ago

CharlesHawkins commented 3 years ago

Cxmon allocates a 16-byte buffer for its prompt string, but the length of the . address it prints is dependent on host pointer size so the string ("[0000000000000000]-> ") is 22 bytes long on 64-bit hosts. On x86_64 Ubuntu 20.10 this causes cxmon to crash when I try to invoke it:

Basilisk II V1.0 by Christian Bauer et al.
Reading ROM file...
WARNING: Cannot open /dev/cdrom (No such file or directory)
Using SDL/pulseaudio audio output
Using SDL_Renderer driver: opengl
^CD0: 00000000 D1: 00000000 D2: 000002ea D3: 007de5ac
D4: 0000ffff D5: 00000000 D6: 0000005a D7: 007e0000
A0: 007e43c0 A1: 007e43c0 A2: 007dd6b6 A3: 00000000
A4: 00000000 A5: 007ec058 A6: 007d7f7e A7: 007d7f62
USP=00000000 ISP=007d7f62 MSP=00000000 VBR=00000000
T=00 S=1 M=0 X=1 N=0 Z=1 V=0 C=0 IMASK=0
FP0: nan FP1: nan FP2: nan FP3: nan
FP4: nan FP5: nan FP6: nan FP7: nan
N=0 Z=0 I=0 NAN=0
00063136: 7137 4e71 41e8 0080 2010 EMULOP.L #$00000037
next PC: 00063138
*** buffer overflow detected ***: terminated
Aborted (core dumped)

It's presumably overflowing on other 64-bit hosts as well but seemingly not all of them detect it and crash like this (e.g. no crash when I try it on x86_64 macOS Big Sur though the prompt is still 22 bytes long). Just upping the buffer size a little (I made it 32 bytes) seems to fix it.

diff --git a/cxmon/src/mon.cpp b/cxmon/src/mon.cpp
index 29a2457a..246369ea 100644
--- a/cxmon/src/mon.cpp
+++ b/cxmon/src/mon.cpp
@@ -1254,7 +1254,7 @@ void mon(int argc, const char **argv)
        char *cmd = NULL;
        while (!done) {
                if (interactive) {
-                       char prompt[16];
+                       char prompt[32];
                        sprintf(prompt, "[%0*lx]-> ", int(2 * sizeof(mon_dot_address)), mon_dot_address);
                        read_line(prompt);
                        if (!input) {

Now it seems like it might make more sense if the length of addresses it prints in the prompt, memory dumps etc. were sized based on the emulated cpu at least when invoked from BII/SS (i.e. 8 hex digits long representing 4 bytes) but this fix at least makes it not overflow the buffer and crash.

rickyzhang82 commented 3 years ago

In 64 bit box, uintptr is defined as unsigned long -- 64bit. In hex, 64 bit unsigned log with 16 width and 0 as prefix should occupy 16 bytes ASCII. Yup, it is stack overflow in 64 bit.

Good catch. Do you want to send a PR and use snprintf?