On Friday 20 June 2008 17:42, walter harms wrote:
> Hello Denys,
> can you please look at "vi.c coredump" mail ?
>
> I found that that stupid_insert() can return NULL (what is never checked)
> and therefore can crash vi. it is not a security issue but very annoying.
> unfortunately is that a core function and i feel not good to add simple
> checks without understand what they may cause.
You are assuming that I know busybox vi.c :)
I don't. Nobody knows it :)
Okay.
static char *stupid_insert(char * p, char c) // stupidly insert the char c at
'p'
{
p = text_hole_make(p, 1);
if (p != 0) {
*p = c;
file_modified++; // has the file been modified
p++;
}
return p;
}
Neat function, yes. it will silently fail to insert char if p == NULL.
How nice. I bet callers don't expect that!
// open a hole in text[]
static char *text_hole_make(char * p, int size) // at "p", make a 'size' byte
hole
{
char *src, *dest;
int cnt;
if (size <= 0)
goto thm0;
src = p;
dest = p + size;
cnt = end - src; // the rest of buffer
if ( ((end + size) >= (text + text_size)) // TODO: realloc here
|| memmove(dest, src, cnt) != dest) {
status_line_bold("can't create room for new characters");
p = NULL;
goto thm0;
}
memset(p, ' ', size); // clear new hole
end += size; // adjust the new END
file_modified++; // has the file been modified
thm0:
return p;
}
More coolness. "if memmove(dest, src, cnt) != dest ..." - what?!
Since when memmove can return something else than dest?
Wait. Look closely. src, dest, cnt are all NEVER USED! 8]
Superfluous file_modified++ in stupid_insert()...
Oh well.
Try this patch please.
--
vda
diff -d -urpN busybox.8/editors/vi.c busybox.9/editors/vi.c
--- busybox.8/editors/vi.c 2008-06-17 17:53:29.000000000 +0200
+++ busybox.9/editors/vi.c 2008-06-20 21:12:14.000000000 +0200
@@ -451,9 +451,7 @@ static int init_text_buffer(char *fn)
/* allocate/reallocate text buffer */
free(text);
- text_size = size * 2;
- if (text_size < 10240)
- text_size = 10240; // have a minimum size for new files
+ text_size = size + 10240;
screenbegin = dot = end = text = xzalloc(text_size);
if (fn != current_filename) {
@@ -970,7 +968,7 @@ static void colon(char *buf)
// if the insert is before "dot" then we need to update
if (q <= dot)
dot += ch;
- file_modified++;
+ file_modified = 1;
}
} else if (strncasecmp(cmd, "rewind", i) == 0) { // rewind cmd line args
if (file_modified && !useforce) {
@@ -1611,7 +1609,7 @@ static char *char_insert(char * p, char
c = get_one_char();
*p = c;
p++;
- file_modified++; // has the file been modified
+ file_modified = 1; // has the file been modified
} else if (c == 27) { // Is this an ESC?
cmd_mode = 0;
cmdcnt = 0;
@@ -1654,12 +1652,9 @@ static char *char_insert(char * p, char
static char *stupid_insert(char * p, char c) // stupidly insert the char c at 'p'
{
p = text_hole_make(p, 1);
- if (p != 0) {
- *p = c;
- file_modified++; // has the file been modified
- p++;
- }
- return p;
+ *p = c;
+ //file_modified = 1; - done by text_hole_make()
+ return p + 1;
}
static int find_range(char ** start, char ** stop, char c)
@@ -1842,24 +1837,21 @@ static void showmatching(char *p)
// open a hole in text[]
static char *text_hole_make(char * p, int size) // at "p", make a 'size' byte hole
{
- char *src, *dest;
- int cnt;
-
if (size <= 0)
- goto thm0;
- src = p;
- dest = p + size;
- cnt = end - src; // the rest of buffer
- if ( ((end + size) >= (text + text_size)) // TODO: realloc here
- || memmove(dest, src, cnt) != dest) {
- status_line_bold("can't create room for new characters");
- p = NULL;
- goto thm0;
+ return p;
+ end += size; // adjust the new END
+ if (end >= (text + text_size)) {
+ char *new_text;
+ text_size += end - (text + text_size) + 10240;
+ new_text = xrealloc(text, text_size);
+ screenbegin = new_text + (screenbegin - text);
+ dot = new_text + (dot - text);
+ end = new_text + (end - text);
+ p = new_text + (p - text);
+ text = new_text;
}
memset(p, ' ', size); // clear new hole
- end += size; // adjust the new END
- file_modified++; // has the file been modified
- thm0:
+ file_modified = 1;
return p;
}
@@ -1885,16 +1877,14 @@ static char *text_hole_delete(char * p,
goto thd0;
if (src >= end)
goto thd_atend; // just delete the end of the buffer
- if (memmove(dest, src, cnt) != dest) {
- status_line_bold("can't delete the character");
- }
+ memmove(dest, src, cnt);
thd_atend:
end = end - hole_size; // adjust the new END
if (dest >= end)
dest = end - 1; // make sure dest in below end-1
if (end <= text)
dest = end = text; // keep pointers valid
- file_modified++; // has the file been modified
+ file_modified = 1; // has the file been modified
thd0:
return dest;
}
@@ -2412,7 +2402,7 @@ static int file_insert(const char * fn,
status_line_bold("cannot read all of file \"%s\"", fn);
}
if (cnt >= size)
- file_modified++;
+ file_modified = 1;
close(fd);
fi0:
#if ENABLE_FEATURE_VI_READONLY
@@ -3547,7 +3537,7 @@ static void do_cmd(char c)
dot_end(); // move to NL
if (dot < end - 1) { // make sure not last char in text[]
*dot++ = ' '; // replace NL with space
- file_modified++;
+ file_modified = 1;
while (isblank(*dot)) { // delete leading WS
dot_delete();
}
@@ -3750,7 +3740,7 @@ static void do_cmd(char c)
c1 = get_one_char(); // get the replacement char
if (*dot != '\n') {
*dot = c1;
- file_modified++; // has the file been modified
+ file_modified = 1; // has the file been modified
}
end_cmd_q(); // stop adding to q
break;
@@ -3795,10 +3785,10 @@ static void do_cmd(char c)
} // repeat cnt
if (islower(*dot)) {
*dot = toupper(*dot);
- file_modified++; // has the file been modified
+ file_modified = 1; // has the file been modified
} else if (isupper(*dot)) {
*dot = tolower(*dot);
- file_modified++; // has the file been modified
+ file_modified = 1; // has the file been modified
}
dot_right();
end_cmd_q(); // stop adding to q
_______________________________________________
busybox mailing list
[email protected]
http://busybox.net/cgi-bin/mailman/listinfo/busybox
