| Message ID | 1509211918-14829-1-git-send-email-u9012063@gmail.com |
|---|---|
| Headers | show |
| Series | Fix clang static analysis null pointer bugs. | expand |
I've reviewed everything except for patches 1, 6, 7, and 9. I'm not very familiar with those areas of code and do not feel as comfortable giving an ACK to those. On Sat, Oct 28, 2017 at 12:32 PM William Tu <u9012063@gmail.com> wrote: > Before the patch, the scan-build reports 46 bugs found. > The patch series fix the clang static checker bug group: > "Argument with nonnull attribute passed null" > Most of the fixes are adding "ovs_assert" to tell the checker > that the pointer won't be null. Now the bugs count reduces to 35. > Tested it by doing "make clang-analyze" > > William Tu (11): > dp-packet: fix possible null pointer argument > ovn-controller: Fix possible null pointer in memcmp. > ovs-appctl: Fix possible null pointer argument. > lib/process: Fix possible null pointer argument. > ovs-ofctl: Fix possible null pointer. > packets: Fix possible null pointer argument to memmove. > packets: Fix possible null pointer to memcmp. > ovn-sbctl: Fix possible null pointer to qsort. > tnl-neigh-cache: Fix possible null pointer. > lib: Fix possible null pointer to execvp. > ovs-testcontroller: Fix possible null pointer to atoi. > > lib/dp-packet.c | 1 + > lib/packets.c | 2 ++ > lib/process.c | 2 ++ > lib/tnl-neigh-cache.c | 3 +++ > ovn/controller/pinctrl.c | 1 + > ovn/utilities/ovn-sbctl.c | 1 + > utilities/ovs-appctl.c | 1 + > utilities/ovs-ofctl.c | 1 + > utilities/ovs-testcontroller.c | 2 ++ > 9 files changed, 14 insertions(+) > > -- > 2.7.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Sat, Oct 28, 2017 at 10:31:47AM -0700, William Tu wrote: > Before the patch, the scan-build reports 46 bugs found. > The patch series fix the clang static checker bug group: > "Argument with nonnull attribute passed null" > Most of the fixes are adding "ovs_assert" to tell the checker > that the pointer won't be null. Now the bugs count reduces to 35. > Tested it by doing "make clang-analyze" Thanks for taking a look at all those warnings. In general, I'm not a fan of adding assertions for nonnull pointers that are dereferenced soon after. This kind of an assertion doesn't really help much, because it just converts a SIGSEGV signal into a SIGABRT signal. I agree that it makes sense to add them when the assertion is long before the dereference, or if the pointer should always be nonnull but is not necessarily dereferenced. I don't think it should be a goal to eliminate all clang-analyzer warnings. clang-analyzer simply doesn't have enough context in many cases, so it gives incorrect warnings. I'll make some more specific comments on individual patches.