Page MenuHomeFreeBSD
Differential D5245

PVS Static Analysis
Needs ReviewPublic
Actions

Authored by wblock on Feb 10 2016, 6:34 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Jan 26, 5:34 PM
Unknown Object (File)
Thu, Jan 23, 8:50 AM
Unknown Object (File)
Sun, Jan 12, 1:09 PM
Unknown Object (File)
Nov 11 2024, 8:10 AM
Unknown Object (File)
Nov 10 2024, 7:10 PM
Unknown Object (File)
Oct 18 2024, 7:59 AM
Unknown Object (File)
Oct 14 2024, 9:47 PM
Unknown Object (File)
Oct 12 2024, 8:43 PM
View All Files
Subscribers
araujo
cem
cy
emaste
• hselasky
imp
kevlo
View All 16 Subscribers
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Commits
rS296299: Remove redundant NULL pointer comparison.
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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
• hselasky added a comment.Feb 14 2016, 7:21 AM

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.

rpokala added a subscriber: rpokala.Feb 15 2016, 6:37 AM
araujo added a subscriber: araujo.Feb 16 2016, 2:46 PM
rmacklem added a subscriber: rmacklem.Feb 17 2016, 12:11 AM
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 mentioned this in rS295708: Address a warning reported by D5245 / PVS..Feb 17 2016, 5:53 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)

Addressed in https://svnweb.freebsd.org/changeset/base/295708.

856 ↗(On Diff #13188)

Adressed in https://svnweb.freebsd.org/changeset/base/295708.

tuexen mentioned this in rS295709: Code cleanup which will silence a warning in PVS / D5245..Feb 17 2016, 6:04 PM
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 mentioned this in rS295710: Add protection code for issues reported by PVS / D5245..Feb 17 2016, 6:13 PM
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)

Addressed in https://svnweb.freebsd.org/changeset/base/295710.

850 ↗(On Diff #13188)

Addressed in https://svnweb.freebsd.org/changeset/base/295710.

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 mentioned this in rS295771: Fix reporting of mapped addressed in getpeername() and getsockname() for.Feb 18 2016, 9:05 PM
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)

False positive, but addressed in https://svnweb.freebsd.org/base?view=revision&revision=295759

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.

rmacklem added a comment.Feb 19 2016, 12:07 AM

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.

tuexen mentioned this in rS296052: MFC r295549:.Feb 26 2016, 11:07 AM
Closed by commit rS296299: Remove redundant NULL pointer comparison. (authored by cy). · Explain WhyMar 2 2016, 3:29 AM
This revision was automatically updated to reflect the committed changes.
cy mentioned this in rS296299: Remove redundant NULL pointer comparison..
wblock reopened this revision.Mar 2 2016, 1:14 PM
• hselasky mentioned this in rS296443: MFC r295608:.Mar 7 2016, 9:35 AM
cy mentioned this in rS296923: MFC r296299 - Remove redundant NULL pointer comparison..Mar 16 2016, 1:48 AM
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.

GitHub <noreply@github.com> mentioned this in rGf091db924883: free_blocks(): Fix reports from 2016 PVS Studio FreeBSD report.Jan 25 2023, 2:47 PM

Revision Contents
Changeset List

PathSize
head/
sys/
contrib/
ipfilter/
netinet/
4 lines

Diff 13969

View Options

head/sys/contrib/ipfilter/netinet/ip_ftp_pxy.c

Loading...