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);
This one uses a size of zero to get the required size. That's fine. > message.c: str_l = vim_vsnprintf(str + len, space, fmt, ap, NULL); > message.c: str_l = vim_vsnprintf(str, str_m, fmt, ap, NULL); These are the internal implementations of vim_snprintf() and vim_snprintf_add(). The return value is just passed on. Also fine. > >> * 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; Yep, all is fine. -- Computers make very fast, very accurate, mistakes. /// Bram Moolenaar -- [email protected] -- http://www.Moolenaar.net \\\ /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\ an exciting new programming language -- http://www.Zimbu.org /// \\\ help me help AIDS victims -- http://ICCF-Holland.org /// -- -- 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.
