hi denys,
you patch prevents the crashs i had.
Since no one understands vi.c realy we can add some more comments like:
/*
text=start of buffer
end=end of user_input
text_size= size of buffer
the new text size is calculated:
text_size += end - (text + text_size) + 10240;
text + text_size= Address for End Of Buffer
end-EOB = additional needed size
The systems add 10K (10240) additional space
perhaps this 10240 should be a #define ?
*/
static char *text_hole_make(char * p, int size) // at "p", make a 'size' byte
hole
{
if (size <= 0)
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
file_modified = 1;
return p;
}
re,
wh
Denys Vlasenko wrote:
> 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
_______________________________________________
busybox mailing list
[email protected]
http://busybox.net/cgi-bin/mailman/listinfo/busybox
