On 13/05/2009 20:26, Dominique Pellé wrote:
> Bram Moolenaar wrote:
>
>> Dominique Pelle wrote:
>>
>>> --001485f04092b96eb10469cb5cc9
>>> Content-Type: text/plain; charset=ISO-8859-1
>>> Content-Transfer-Encoding: quoted-printable
>>>
>>> Bram Moolenaar wrote:
>>>
>>>> Patch 7.2.167
>>>> Problem: Splint doesn't work well for checking the code.
>>>> Solution: Add splint arguments in the Makefile. Exclude some code from
>>>> splint that it can't handle. Tune splint arguments to give
>>>> reasonable errors. Add a filter for removing false warnings
>>>> from
>>>> splint output. Many small changes to avoid warnings. More to
>>>> follow...
>>>> Files: Filelist, src/Makefile, src/buffer.c, src/charset.c,
>>>> src/cleanlint.vim, src/digraph.c, src/edit.c, src/ex_cmds.c,
>>>> src/globals.h, src/ops.c, src/os_unix.c, src/os_unix.h,
>>>> src/proto/buffer.pro, src/proto/edit.pro, src/screen.c,
>>>> src/structs.h
>>> Patch 7.2.167 introduces a regression: test40 fails on one of
>>> my machine (Linux x86_64) but succeeds on another (Linux x86).
>>>
>>> Running test40 with valgrind reveals this error (on both machines):
>>>
>>> ==10911== Conditional jump or move depends on uninitialised value(s)
>>> ==10911== at 0x80521F4: buf_same_ino (buffer.c:2944)
>>> ==10911== by 0x805212E: otherfile_buf (buffer.c:2905)
>>> ==10911== by 0x8050F21: buflist_findname_stat (buffer.c:2013)
>>> ==10911== by 0x80502F9: buflist_new (buffer.c:1519)
>>> ==10911== by 0x8092A3F: do_ecmd (ex_cmds.c:3308)
>>> ==10911== by 0x80A96DF: do_exedit (ex_docmd.c:7584)
>>> ==10911== by 0x80A9356: ex_edit (ex_docmd.c:7479)
>>> ==10911== by 0x80A1EE3: do_one_cmd (ex_docmd.c:2622)
>>> ==10911== by 0x809F763: do_cmdline (ex_docmd.c:1096)
>>> ==10911== by 0x8124EB0: nv_colon (normal.c:5227)
>>> ==10911== by 0x811E6C8: normal_cmd (normal.c:1189)
>>> ==10911== by 0x80E1745: main_loop (main.c:1180)
>>> ==10911== by 0x80E1292: main (main.c:939)
>>>
>>> src/buffer.c:
>>>
>>> 2939 static int
>>> 2940 buf_same_ino(buf, stp)
>>> 2941 buf_T *buf;
>>> 2942 struct stat *stp;
>>> 2943 {
>>> 2944 return (buf->b_dev >= 0
>>> 2945 && stp->st_dev == buf->b_dev
>>> 2946 && stp->st_ino == buf->b_ino);
>>> 2947 }
>>> 2948 #endif
>>>
>>> Test 'buf->b_dev >= 0' at buffer.c:2940 is incorrect because
>>> type of b_dev field is now 'dev_t' which appears to be
>>> unsigned so test is always true (prior to patch 7.2.167,
>>> type was 'int'). So Vim accesses stp->st_ino which is
>>> uninitialized.
>>>
>>> Attached patch fixes the problem. All tests are OK after
>>> applying attached patch.
>> Thanks for pinpointing this bug. So there must be a place where -1 is
>> assigned to b_dev, assuming that this will never be a valid value. I'm
>> actually not sure this is the case.
>
> There are a couple of places where Vim uses -1 for b_dev or
> st_dev fields:
>
> $ egrep '_dev.*-1' *.c
> buffer.c: st.st_dev = (dev_T)-1;
> buffer.c: if (st.st_dev == (dev_T)-1)
> buffer.c: buf->b_dev = -1;
> buffer.c: st.st_dev = (dev_T)-1;
> buffer.c: st.st_dev = (dev_T)-1;
> buffer.c: st.st_dev = (dev_T)-1;
> buffer.c: if (st.st_dev == (dev_T)-1)
> buffer.c: buf->b_dev = -1;
> buffer.c: if (buf->b_dev == (dev_t)-1 || mch_stat((char
> *)ffname, &st) < 0)
> buffer.c: st.st_dev = (dev_T)-1;
> buffer.c: buf->b_dev = -1;
> buffer.c: return (buf->b_dev != (dev_t)-1
> ex_cmds2.c: ((stat_ok && si->sn_dev != -1)
> ex_cmds2.c: si->sn_dev = -1;
> fileio.c: else if (buf->b_dev == (dev_t)-1)
> misc2.c: int ffv_dev; /* device number (-1
> if not set) */
> misc2.c: vp->ffv_dev = -1;
>
>
>> The problem is that splint gives so many warnings for assigning an int
>> to an unsigned that I ignore that warning.
>>
>> splint turns out to be not a good tool. With the GTK GUI enabled it
>> runs out of memory. I had to change a few source files to avoid it from
>> chocking on non-ASCII characters. It doesn't understand an array of
>> pointers.
>
> I agree. I'm not a fan of splint either. I once tried it but the
> amount of noise or spurious errors was just too high to use it
> systematically. Using stricter warning options of the C compiler
> gives better results on my opinion than using splint.
>
>> Does someone know a better lint than splint? Unfortunately the lint I
>> was using on FreeBSD doesn't appear to be available for Linux. It
>> wasn't great but I least I could use it for some basic sanity checking.
>
> The only good static analyzer I've heard of is coverity. I once saw
> a demo and it looked very good, but it's also very expensive. I see
> in the link below that Vim has already been checked with coverity
> (no idea which version of Vim was tested):
>
> http://scan.coverity.com/rung1.html
>
> I don't know when or if the coverity scan will be rerun. I see
> that the last run was in March 2007:
>
> http://scan.coverity.com/ladder.html#future
>
> That was more than 2 years ago. Maybe the coverity scan
> program is dead. I hope not.
Coverity is till going strong. AFAIK it does free audits for open source -
http://www.internetnews.com/dev-news/article.php/3815381
TTFN
Mike
--
Intelligence is like a river, the deeper it is, the less noise it makes.
--~--~---------~--~----~------------~-------~--~----~
You received this message from the "vim_dev" maillist.
For more information, visit http://www.vim.org/maillist.php
-~----------~----~----~----~------~----~------~--~---
