Bram Moolenaar wrote: > Dominique Pellé wrote: > >> afl-fuzz found another memory error in vim-7.4.1082 (and older). >> Using the attached non sensical 'crash.vim' file: >> >> $ valgrind vim -u NONE -N -S crash.vim 2> log >> >> And log file contains: >> >> ==15151== Memcheck, a memory error detector >> ==15151== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al. >> ==15151== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info >> ==15151== Command: ./vim -u NONE -N -S crash.vim >> ==15151== >> ==15151== Invalid write of size 1 >> ==15151== at 0x409847: buflist_list (buffer.c:2801) >> ==15151== by 0x45FECD: do_one_cmd (ex_docmd.c:2962) >> ==15151== by 0x45B9B0: do_cmdline (ex_docmd.c:1133) >> ==15151== by 0x459AD1: do_source (ex_cmds2.c:3396) >> ==15151== by 0x4592A3: cmd_source (ex_cmds2.c:3005) >> ==15151== by 0x45FECD: do_one_cmd (ex_docmd.c:2962) >> ==15151== by 0x45B9B0: do_cmdline (ex_docmd.c:1133) >> ==15151== by 0x5B9EBC: exe_commands (main.c:2928) >> ==15151== by 0x5B9EBC: main (main.c:962) >> ==15151== Address 0x759d0e4 is 20 bytes after a block of size 1,040 >> in arena "client" >> ==15151== >> ==15151== Invalid write of size 1 >> ==15151== at 0x4C2F673: memcpy@GLIBC_2.2.5 (in >> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) >> ==15151== by 0x4AFCD6: vim_vsnprintf (message.c:4152) >> ==15151== by 0x4B51CC: vim_snprintf (message.c:4106) >> ==15151== by 0x409968: buflist_list (buffer.c:2803) >> ==15151== by 0x45FECD: do_one_cmd (ex_docmd.c:2962) >> ==15151== by 0x45B9B0: do_cmdline (ex_docmd.c:1133) >> ==15151== by 0x459AD1: do_source (ex_cmds2.c:3396) >> ==15151== by 0x4592A3: cmd_source (ex_cmds2.c:3005) >> ==15151== by 0x45FECD: do_one_cmd (ex_docmd.c:2962) >> ==15151== by 0x45B9B0: do_cmdline (ex_docmd.c:1133) >> ==15151== by 0x5B9EBC: exe_commands (main.c:2928) >> ==15151== by 0x5B9EBC: main (main.c:962) >> ==15151== Address 0x759d0e5 is 21 bytes after a block of size 1,040 >> in arena "client" >> ==15151== >> >> The problem happens because code in buffer.c >> wrongly assumes that when vim_snprintf(...) truncates its output, >> it returns the truncated number of bytes. This is incorrect as it >> returns the number of bytes that would have been written if there >> was no truncation. See man snprintf(...): >> >> === BEGIN [man snprintf] === >> If the output was truncated due to this limit then the return >> value is the number of characters (excluding the terminating >> null byte) which would have been written to the final >> string if enough space had been available. Thus, a return >> value of size or more means that the output was truncated. >> (See also below under NOTES.) >> ...snip... >> NOTES >> ..snip... >> The glibc implementation of the functions snprintf() and vsnprintf() >> conforms to the C99 standard, that is, behaves as described >> above, since glibc version 2.1. Until glibc 2.0.6 they would >> return -1 when the output was truncated. >> ==== END [man snprintf] === > > Yes, the comment above vim_snprintf() also states this. > >> Attached patch fixes the bug. However: > > Thanks! > >> * I wonder whether there are other similar bugs elsewhere. >> I see that returned value of vim_snprintf(...) is used in a >> few places which look suspicious in message.c and >> eval.c > > I don't see another case where the return value of vim_snprintf is used. > Where did you see that?
I see at least 4 places. My patch address the one in buffer.c. I did not have time yet to look at others (lacking spare time...): $ grep '=.*nprintf' *.c buffer.c: len = vim_snprintf((char *)IObuff, IOSIZE - 20, "%3d%c%c%c%c%c \"%s\"", eval.c: len = vim_vsnprintf(NULL, 0, fmt, ap, argvars + 1); message.c: str_l = vim_vsnprintf(str + len, space, fmt, ap, NULL); message.c: str_l = vim_vsnprintf(str, str_m, fmt, ap, NULL); >> * also, since old version of glibc could return -1, there might >> be a portability bug there too. I'm not sure whether configure >> checks for that. > > We don't use snprintf() from the library, only sprintf(). Ah. I was wrongly assuming that it was implemented with the C library snprintf(...) or vsnprintf(...). Anyway, the patch I sent is correct, as vim_snprintf(...) has this comment in message.c (as you said): 4957 /* Return the number of characters formatted (excluding trailing nul 4958 * character), that is, the number of characters that would have been 4959 * written to the buffer if it were large enough. */ 4960 return (int)str_l; Regards Dominique -- -- You received this message from the "vim_dev" maillist. Do not top-post! Type your reply below the text you are replying to. For more information, visit http://www.vim.org/maillist.php --- You received this message because you are subscribed to the Google Groups "vim_dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
