--- Begin Message ---
- To: submit@bugs.debian.org
- Subject: spim: syscall read_string is broken by design
- From: Akim Demaille <akim@epita.fr>
- Date: Fri, 28 Feb 2003 08:50:44 +0100
- Message-id: <E18ofI4-0001aq-00@nostromo.lrde.epita.fr>
Package: spim
Version: 6.5-1
Severity: important
The semantics of read_string is badly broken, as is that of read_char:
there is no means to know when EOF is met. It seems that the author
means \0 to stand for EOF, which is not satisfying. But worse yet:
the intend semantics is not implemented properly, which results in the
following incorrect behavior:
~/src/spim % cat spim-bug-report.s nostromo 8:36
.data
getchbuf:
.space 200
.text
main:
la $a0, getchbuf
li $a1, 200
li $v0, 8
syscall
la $a0, getchbuf
li $v0, 4
syscall
li $v0, 10
syscall
~/src/spim % echo -n foo | spim -file spim-bug-report.s nostromo 8:37
SPIM Version 6.5 of January 4, 2003
Copyright 1990-2003 by James R. Larus (larus@cs.wisc.edu).
All Rights Reserved.
See the file README for a full copyright notice.
Loaded: /usr/lib/spim/trap.handler
fooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo
Pay special attention to the -n in the echo command. The problem
exhibits itself more easily this way, but even inputs with \n in it
are vulnerable.
I suggest the following patch, which does fix the issue:
~/src/spim % diff spim.c~ spim.c -u nostromo Err 1
--- spim.c~ 1999-01-08 01:33:34.000000000 +0100
+++ spim.c 2003-02-28 08:32:02.000000000 +0100
@@ -1,7 +1,7 @@
/* SPIM S20 MIPS simulator.
Terminal interface for SPIM simulator.
- Copyright (C) 1990-1998 by James Larus (larus@cs.wisc.edu).
+ Copyright (C) 1990-1998, 2003 by James Larus (larus@cs.wisc.edu).
ALL RIGHTS RESERVED.
Changes for DOS and Windows versions by David A. Carley (dac@cs.wisc.edu)
@@ -927,7 +927,10 @@
while (1 < str_size) /* Reserve space for null */
{
char buf[1];
- read ((int) console_in.i, buf, 1); /* Not in raw mode! */
+
+ /* Not in raw mode! */
+ if (!read ((int) console_in.i, buf, 1))
+ break;
*ptr ++ = buf[0];
str_size -= 1;
But if I can make a suggestion, I would vote for read_string to return
in $a0 the number of characters read. It corresponds to the following
longer patch:
------------------------------
--- spim.c.orig 2003-02-28 08:41:12.000000000 +0100
+++ spim.c 2003-02-28 08:41:59.000000000 +0100
@@ -1,7 +1,7 @@
/* SPIM S20 MIPS simulator.
Terminal interface for SPIM simulator.
- Copyright (C) 1990-1998 by James Larus (larus@cs.wisc.edu).
+ Copyright (C) 1990-1998, 2003 by James Larus (larus@cs.wisc.edu).
ALL RIGHTS RESERVED.
Changes for DOS and Windows versions by David A. Carley (dac@cs.wisc.edu)
@@ -904,10 +904,10 @@
/* Simulate the semantics of fgets (not gets) on Unix file. */
#ifdef __STDC__
-void
+int
read_input (char *str, int str_size)
#else
-void
+int
read_input (str, str_size)
char *str;
int str_size;
@@ -927,7 +927,10 @@
while (1 < str_size) /* Reserve space for null */
{
char buf[1];
- read ((int) console_in.i, buf, 1); /* Not in raw mode! */
+
+ /* Not in raw mode! */
+ if (!read ((int) console_in.i, buf, 1))
+ break;
*ptr ++ = buf[0];
str_size -= 1;
@@ -941,6 +944,8 @@
if (restore_console_to_program)
console_to_program ();
+
+ return ptr - str;
}
--- spim.h.orig 2003-02-28 08:41:21.000000000 +0100
+++ spim.h 2003-02-28 08:42:14.000000000 +0100
@@ -1,7 +1,7 @@
/* SPIM S20 MIPS simulator.
Definitions for the SPIM S20.
- Copyright (C) 1990-1998 by James Larus (larus@cs.wisc.edu).
+ Copyright (C) 1990-1998, 2003 by James Larus (larus@cs.wisc.edu).
ALL RIGHTS RESERVED.
Changes for DOS and Windows versions by David A. Carley (dac@cs.wisc.edu)
@@ -231,7 +231,7 @@
void fatal_error (char *fmt, ...);
char get_console_char (void);
void put_console_char (char c);
-void read_input (char *str, int n);
+int read_input (char *str, int n);
int* run_error (char *fmt, ...);
void write_output (port, char *fmt, ...);
#else
--- mips-syscall.c.orig 2003-02-28 08:43:39.000000000 +0100
+++ mips-syscall.c 2003-02-28 08:44:06.000000000 +0100
@@ -2,7 +2,7 @@
Execute SPIM syscalls, both in simulator and bare mode.
Execute MIPS syscalls in bare mode, when running on MIPS systems.
- Copyright (C) 1990-1998 by James Larus (larus@cs.wisc.edu).
+ Copyright (C) 1990-1998, 2003 by James Larus (larus@cs.wisc.edu).
ALL RIGHTS RESERVED.
Improved by Emin Gun Sirer.
@@ -458,7 +458,8 @@
case READ_STRING_SYSCALL:
{
- read_input ( (char *) MEM_ADDRESS (R[REG_A0]), R[REG_A1]);
+ R[REG_RES] =
+ read_input ( (char *) MEM_ADDRESS (R[REG_A0]), R[REG_A1]);
data_modified = 1;
break;
}
------------------------------
Which behaves like properly, according to the following test case:
------------------------------
~/src/spim % cat spim-bug-report.s nostromo 8:46
.data
getchbuf:
.space 200
.text
main:
la $a0, getchbuf
li $a1, 200
li $v0, 8
syscall
move $a0, $v0
li $v0, 1
syscall
la $a0, getchbuf
li $v0, 4
syscall
li $v0, 10
syscall
~/src/spim % echo -n foo | ./spim -file spim-bug-report.s; echo nostromo 8:48
SPIM Version 6.2 of January 11, 1999
Copyright 1990-1998 by James R. Larus (larus@cs.wisc.edu).
All Rights Reserved.
See the file README for a full copyright notice.
Loaded: /usr/local/share/spim/trap.handler
3foo
------------------------------
Of course, the documentation will have to be updated.
PS/ '\0' should definitely not be EOF :(
-- System Information
Debian Release: testing/unstable
Kernel Version: Linux nostromo 2.4.20 #4 jeu fév 27 14:06:02 CET 2003 i686 unknown unknown GNU/Linux
Versions of the packages spim depends on:
ii libc6 2.3.1-13 GNU C Library: Shared libraries and Timezone
ii libxaw7 4.2.1-5 X Athena widget set library
ii xlibs 4.2.1-5 X Window System client libraries
--- End Message ---