Page MenuHomeFreeBSD

PVS Static Analysis
Needs ReviewPublic

Authored by wblock on Feb 10 2016, 6:34 PM.
This revision needs review, but there are no reviewers specified.

Details

Summary

These are the static analysis results provided by Program Verification Systems. See https://lists.freebsd.org/pipermail/freebsd-doc/2016-February/026371.html

The review was run against r295235 (determined by des@, see https://lists.freebsd.org/pipermail/freebsd-doc/2016-February/026410.html).

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I've fixed all the usb issues in sys/dev/usb . The others are false positives. Thank you!

kevlo added a subscriber: kevlo.Feb 14 2016, 1:45 PM
kp added a subscriber: kp.Feb 14 2016, 3:01 PM
kp added inline comments.
PVS-Studio-log-freebsd.txt
899 ↗(On Diff #13188)

This looks like it might be a problem, added to my TODO list.

901 ↗(On Diff #13188)

Doesn't look like a bug, we don't always allocate new rule, (but if we do it's never NULL because M_WAITOK), so it makes sense to NULL-check it.

902 ↗(On Diff #13188)

This check is just wrong. Yes, we use *m before checking it, but that's because pf_test6() can change it.

903 ↗(On Diff #13188)

Also seems to be wrong, we can only hit the NULL check from places where newpa is allocated.
We can probably get rid of the NULL check, especially considering that free() checks for NULL anyway.

904 ↗(On Diff #13188)

size2 is never NULL, so we can get rid of the NULL check (perhaps adding an assert at the top of the function.

araujo added a subscriber: araujo.Feb 16 2016, 2:46 PM
rmacklem added inline comments.
PVS-Studio-log-freebsd.txt
667 ↗(On Diff #13188)

All of the "tl" ones are bogus. The NFSM_BUILD() and NFSM_DISSECT() macros
set tl to point to a block of memory the size of the argument. It is then incremented
through the block, sometimes skipping some bytes.

681 ↗(On Diff #13188)

The case where it is used it is valid and non-null.

682 ↗(On Diff #13188)

bp is only used when n > 0 and it is always valid and non-null when n > 0.

728 ↗(On Diff #13188)

It is initialized before it is used.

wblock updated this object.Feb 17 2016, 1:54 PM
tuexen added a subscriber: tuexen.Feb 17 2016, 5:57 PM
tuexen added inline comments.
PVS-Studio-log-freebsd.txt
851 ↗(On Diff #13188)
856 ↗(On Diff #13188)
tuexen added inline comments.Feb 17 2016, 6:08 PM
PVS-Studio-log-freebsd.txt
862 ↗(On Diff #13188)

This is not an issue, but the code has been cleaned up such that the warning will not show up anymore.
This is done in https://svnweb.freebsd.org/changeset/base/295709.

tuexen added inline comments.Feb 17 2016, 6:27 PM
PVS-Studio-log-freebsd.txt
846 ↗(On Diff #13188)

False positive. If net != NULL then stcb =! NULL.

847 ↗(On Diff #13188)

False positive.

848 ↗(On Diff #13188)

False positive.

849 ↗(On Diff #13188)
850 ↗(On Diff #13188)
852 ↗(On Diff #13188)

False positive.

853 ↗(On Diff #13188)

False positive.

854 ↗(On Diff #13188)

False positive.

855 ↗(On Diff #13188)

False positive.

857 ↗(On Diff #13188)

False positive.

858 ↗(On Diff #13188)

False positive.

859 ↗(On Diff #13188)

False positive.

860 ↗(On Diff #13188)

False positive.

861 ↗(On Diff #13188)

False positive.

866 ↗(On Diff #13188)

False positive.

867 ↗(On Diff #13188)

False positive.

869 ↗(On Diff #13188)

False positive.

870 ↗(On Diff #13188)

False positive.

871 ↗(On Diff #13188)

False positive.

872 ↗(On Diff #13188)

False positive.

873 ↗(On Diff #13188)

False positive.

874 ↗(On Diff #13188)

False positive.

875 ↗(On Diff #13188)

False positive.

876 ↗(On Diff #13188)

False positive.

tuexen added inline comments.Feb 17 2016, 6:54 PM
PVS-Studio-log-freebsd.txt
863 ↗(On Diff #13188)

False positive.

864 ↗(On Diff #13188)

False positive.

865 ↗(On Diff #13188)

False positive.

868 ↗(On Diff #13188)

False positive.

markj added a subscriber: markj.Feb 17 2016, 8:22 PM
markj added inline comments.
PVS-Studio-log-freebsd.txt
37 ↗(On Diff #13188)

The comment below tries to explain it: the check is bogus and is intended to prevent the compiler from making the dtrace_getarg() call a tail call. So this is a false positive.

I wonder if that trick is really reliable though. The check is only false if undefined behaviour has been invoked.

38 ↗(On Diff #13188)

False positive. panic() doesn't return.

39 ↗(On Diff #13188)

False positive; nspec > 0 => spec != NULL.

40 ↗(On Diff #13188)

False positive; the code is assuming that enab->dten_ndesc > 0 implies that enab->dten_desc != NULL.

41 ↗(On Diff #13188)

It does seem excessive, but I don't see why it might be wrong. It's also an assertion condition.

marius added a subscriber: marius.Feb 17 2016, 10:52 PM
marius added inline comments.
PVS-Studio-log-freebsd.txt
269 ↗(On Diff #13188)

False positive

558 ↗(On Diff #13188)

False positive

tuexen added inline comments.Feb 18 2016, 9:09 PM
PVS-Studio-log-freebsd.txt
883 ↗(On Diff #13188)

The bug reported can not happen, since the function is not really working as expected.
This is fixed in https://svnweb.freebsd.org/changeset/base/295771.

884 ↗(On Diff #13188)

The bug reported can not happen, since the function is not really working as expected.
This is fixed in https://svnweb.freebsd.org/changeset/base/295771.

imp added inline comments.Feb 18 2016, 9:41 PM
PVS-Studio-log-freebsd.txt
29 ↗(On Diff #13188)
imp added inline comments.Feb 18 2016, 9:53 PM
PVS-Studio-log-freebsd.txt
30 ↗(On Diff #13188)

False positive. We always have a bp for a DELETE ccb, but the code is twisty enough that the static analyzer missed it.

32 ↗(On Diff #13188)

False positive. serial_buf is always != NULL when have_serial is true, which we check in this loop.

cy added a subscriber: cy.Feb 18 2016, 11:36 PM
rmacklem added inline comments.Feb 18 2016, 11:48 PM
PVS-Studio-log-freebsd.txt
732 ↗(On Diff #13188)

It is initialized before used.

734 ↗(On Diff #13188)

It is initialized before being used.

735 ↗(On Diff #13188)

The if and else do the same thing, which is silly but not broken. I believe this
was inherited from the old NFS client, so I left the code that way, although I
did notice this.

736 ↗(On Diff #13188)

This one I don't understand. "iv" is declared local to the function and is used
within the function, so I have no idea why it thinks there is a scope issue?

737 ↗(On Diff #13188)

False positive, since the code checks for nfsd_master_proc != NULL
and then procp == nfsd_master_proc --> procp cannot be NULL.

768 ↗(On Diff #13188)

It is initialized before being used.

787 ↗(On Diff #13188)

It is initialized before being used.

667->788 are all false positives (no bugs identified), although #735 is silly and the
code should probably be cleaned up to take the if/else out someday.

PVS-Studio-log-freebsd.txt
788 ↗(On Diff #13188)

Also a false positive. The only time nd == NULL is when the function is
called with CLOPS_RENEW. This code block isn't executed for CLOPS_RENEW.

wblock changed the visibility from "committers (Project)" to "All Users".Feb 19 2016, 4:08 PM
eadler changed the visibility from "All Users" to "Public (No Login Required)".Feb 19 2016, 4:18 PM
se added a subscriber: se.Feb 19 2016, 4:46 PM
se added inline comments.
PVS-Studio-log-freebsd.txt
359 ↗(On Diff #13188)

Does no harm, but I'll fix it to simplify the loop condition where it occurs.

se added inline comments.Feb 19 2016, 5:24 PM
PVS-Studio-log-freebsd.txt
588 ↗(On Diff #13188)

False positive: baddr != NULL is tested and only possible for vbp != NULL

589 ↗(On Diff #13188)

Looks like a false positive: The value read into cp cannot be NULL unless there was a malloc error during driver attach (which had made the attach fail).

op added a subscriber: op.Feb 19 2016, 8:42 PM
op added inline comments.Feb 19 2016, 9:33 PM
PVS-Studio-log-freebsd.txt
801 ↗(On Diff #13188)

This is false positive, but moving "structsize = sizeof(elf_kinfo_proc_t);" before the check would fix this warning.
Not yet fixed / silenced this warning.

802 ↗(On Diff #13188)

This is false positive, p used as dummy parameter at this call.

trasz added a subscriber: trasz.Feb 25 2016, 12:53 PM
trasz added inline comments.
PVS-Studio-log-freebsd.txt
289 ↗(On Diff #13188)

Hm, no idea what might be wrong here. False positive, perhaps? I'd appreciate if someone took a look at this.

markj added inline comments.Feb 26 2016, 5:30 AM
PVS-Studio-log-freebsd.txt
932 ↗(On Diff #13188)

This one looks like a valid bug: ufs_dir_dd_ino() should set *dd_vp to NULL before returning in all cases.

This revision was automatically updated to reflect the committed changes.
wblock reopened this revision.Mar 2 2016, 1:14 PM
cy added a comment.May 15 2016, 4:25 PM

\sys\contrib\ipfilter\netinet\ip_rpcb_pxy.c (436): error V567: Undefined behavior. The 'p' variable is modified while being used twice between sequence points:

I would have coded this differently, using structs, making the code self documenting. This is OK as coded.

cy added a comment.May 15 2016, 4:28 PM

\sys\contrib\ipfilter\netinet\ip_rpcb_pxy.c (912): error V567: Undefined behavior. The 'p' variable is modified while being used twice between sequence points:

This is OK.

cy added a comment.May 15 2016, 4:30 PM

All these are OK.

\sys\contrib\ipfilter\netinet\ip_rpcb_pxy.c (994): error V567: Undefined behavior. The 'p' variable is modified while being used twice between sequence points.
\sys\contrib\ipfilter\netinet\ip_rpcb_pxy.c (436): error V567: Undefined behavior. The 'p' variable is modified while being used twice between sequence points.
\sys\contrib\ipfilter\netinet\ip_rpcb_pxy.c (912): error V567: Undefined behavior. The 'p' variable is modified while being used twice between sequence points.
\sys\contrib\ipfilter\netinet\ip_rpcb_pxy.c (877): error V567: Undefined behavior. The 'p' variable is modified while being used twice between sequence points.
\sys\contrib\ipfilter\netinet\ip_rpcb_pxy.c (549): error V567: Undefined behavior. The 'p' variable is modified while being used twice between sequence points.
\sys\contrib\ipfilter\netinet\ip_rpcb_pxy.c (438): error V567: Undefined behavior. The 'p' variable is modified while being used twice between sequence points.
\sys\contrib\ipfilter\netinet\ip_rpcb_pxy.c (437): error V567: Undefined behavior. The 'p' variable is modified while being used twice between sequence points.
\sys\contrib\ipfilter\netinet\ip_rpcb_pxy.c (895): error V567: Undefined behavior. The 'p' variable is modified while being used twice between sequence points.
\sys\contrib\ipfilter\netinet\ip_rpcb_pxy.c (891): error V567: Undefined behavior. The 'p' variable is modified while being used twice between sequence points.
\sys\contrib\ipfilter\netinet\ip_rpcb_pxy.c (433): error V567: Undefined behavior. The 'p' variable is modified while being used twice between sequence points.

cy added a comment.May 15 2016, 4:35 PM

More false positives. The scanner failed to detect htons(), htonl(), ntohs() and ntohl().

\sys\contrib\ipfilter\netinet\ip_sync.c (358): error V523: The 'then' statement is equivalent to the 'else' statement.
\sys\contrib\ipfilter\netinet\ip_sync.c (330): error V523: The 'then' statement is equivalent to the 'else' statement.