GNOME Bugzilla – Bug 728320
[review] th/bg728320_coverity: fixes for issues found by coverity
Last modified: 2015-01-12 15:51:03 UTC
Created attachment 274430 [details] covscan result I did a coverity scan based on commit http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=05b5577815381a12b335b0371edd19112b107402 Attached you find the result html.
Please review branch th/bg728320_coverity which fixes all of the issues except the ones in libgsystem and NetworkManager-0.9.9.1/docs/libnm-glib/libnm-glib-scan.c
> all: fix various warnings detected with coverity > padded_buf = g_malloc0 (padded_buf_len); >+ g_assert (padded_buf); The right fix for all the g_malloc0()-related warnings is to make coverity realize that g_malloc0() can't return NULL, by *removing* the checks that appear after g_malloc0() calls in other parts of the source: danw@laptop:NetworkManager (master)> ack -A1 malloc0|grep assert src/settings/plugins/keyfile/writer.c-631- g_assert (tmppath); src/settings/plugins/ifupdown/tests/test-ifupdown.c-51- g_assert (k); src/settings/plugins/ifupdown/tests/test-ifupdown.c-79- g_assert (b); src/settings/plugins/ifupdown/tests/test-ifupdown.c-112- g_assert (e); src/devices/wimax/iwmxsdk.c-143- g_assert (cb); > copy = nm_ip4_address_dup (address); >+ >+ /* coverity[leaked_storage] */ > g_return_val_if_fail (copy != NULL, FALSE); nm_ip4_address_dup() can't return NULL. Just remove the check. Likewise with the fix in nm_setting_ip4_config_add_route() and the ones in nm-setting-ip6-config.c and nm-setting-vlan.c > g_assert_not_reached (); > g_return_val_if_reached ("XXXXX"); >+ /* coverity[unreachable] */ > return "XXXXX"; This is even more ridiculous than it used to be. >- g_return_val_if_fail (username && password, -1); >+ if (!username || !password) { >+ g_return_val_if_reached (-1); >+ return -1; >+ } This seems like the sort of thing that someone (ie, me) would see and fix back to the original form later. Note that it's actually the previously check of username that causes the warning. Maybe reorganize the start of the function to: if (!password) { g_return_val_if_fail (username != NULL, -1); /* pppd is checking pap support; return 1 for supported */ return 1; } >- g_return_val_if_fail (type_data != NULL, NULL); >+ if (!type_data) { >+ g_return_val_if_reached (NULL); >+ return NULL; >+ } I don't understand why this is needed here, and not in so many other places...
(In reply to comment #2) > > all: fix various warnings detected with coverity > > > padded_buf = g_malloc0 (padded_buf_len); > >+ g_assert (padded_buf); > > The right fix for all the g_malloc0()-related warnings is to make coverity > realize that g_malloc0() can't return NULL, by *removing* the checks that > appear after g_malloc0() calls in other parts of the source: > > danw@laptop:NetworkManager (master)> ack -A1 malloc0|grep assert > src/settings/plugins/keyfile/writer.c-631- g_assert (tmppath); > src/settings/plugins/ifupdown/tests/test-ifupdown.c-51- g_assert (k); > src/settings/plugins/ifupdown/tests/test-ifupdown.c-79- g_assert (b); > src/settings/plugins/ifupdown/tests/test-ifupdown.c-112- g_assert (e); > src/devices/wimax/iwmxsdk.c-143- g_assert (cb); > > > copy = nm_ip4_address_dup (address); > >+ > >+ /* coverity[leaked_storage] */ > > g_return_val_if_fail (copy != NULL, FALSE); > > nm_ip4_address_dup() can't return NULL. Just remove the check. Likewise with > the fix in nm_setting_ip4_config_add_route() and the ones in > nm-setting-ip6-config.c and nm-setting-vlan.c > > > g_assert_not_reached (); > > g_return_val_if_reached ("XXXXX"); > >+ /* coverity[unreachable] */ > > return "XXXXX"; > > This is even more ridiculous than it used to be. > > >- g_return_val_if_fail (username && password, -1); > >+ if (!username || !password) { > >+ g_return_val_if_reached (-1); > >+ return -1; > >+ } > > This seems like the sort of thing that someone (ie, me) would see and fix back > to the original form later. > > Note that it's actually the previously check of username that causes the > warning. Maybe reorganize the start of the function to: > > if (!password) { > g_return_val_if_fail (username != NULL, -1); > > /* pppd is checking pap support; return 1 for supported */ > return 1; > } > > >- g_return_val_if_fail (type_data != NULL, NULL); > >+ if (!type_data) { > >+ g_return_val_if_reached (NULL); > >+ return NULL; > >+ } > > I don't understand why this is needed here, and not in so many other places... Pushed a fixup! addressing all remarks. Some places where it seemingly ignored your remark caused the coverity warning to reappear. Especially it does not seem to have any effect of checking the result of g_malloc0 at other places. With the new fixup, still all coverity warnings are solved.
(In reply to comment #3) > Some places where it seemingly ignored your remark caused the coverity warning > to reappear. Especially it does not seem to have any effect of checking the > result of g_malloc0 at other places. Hm... I don't like adding annotations where we don't understand why they're needed. We could be hiding other bugs. And the fact that it only warns about a small percentage of g_malloc0() uses (and no g_malloc(), g_new0(), etc uses) suggests that there's something different about these call sites as opposed to others. Is there a way to get more verbose output? I thought I remember in the past it would point out examples of other places that checked for a NULL return value, which it used as evidence that the function could return NULL... Other places we appear to be checking the results of g_malloc0: - libnm-util/crypto_nss.c:287 and crypto_gnutls.c:224 check "if (output)", but output must be non-NULL - libnm-util/crypto_nss.c:337 does "g_assert (padded_buf)" - libnm-util/crypto_gnutls.c:318 checks "if (padded_buf)", but padded_buf must be non-NULL - libnm-util/crypto.c:424 checks "if (key)", but key must be non-NULL Or maybe it's because of other annotations we already added elsewhere? OK, looking at every coverity annotation in th/bg728320_coverity (not just newly-added ones): libnm-util/nm-utils.c:728 and 757 - This is blatantly dead code; in both cases we already bailed out if adhoc==FALSE. If we want to keep the old code around in case the kernel bugs get fixed in the future, it would be better to just #ifdef out the checks here, rather than coverity-annotating them. libnm-util/nm-utils.c:2129 - coverity[dereference] annotation after g_malloc0(). Shouldn't be needed. It's possible that it's inferring a possible NULL here because src/settings/plugins/ifnet/connection_parser.c:1139 checks that "converted" isn't NULL, even though there's no way it could be NULL there. So maybe removing the check there will make the annotation unnecessary here? libnm-util/nm-setting-vlan.c:264 - it's claiming item can get leaked if item is NULL??? Hm... I wonder if the G_LIKELY() in g_return_val_if_fail() confuses coverity somehow? (Are you building with optimization? G_LIKELY(expr) should expand to just "expr" at -O0.) src/settings/plugins/ifcfg-rh/reader.c:3408 - coverity[dereference] is needed here because we used g_strcmp0() before (lines 3369-3371). We could remove the annotation by changing the earlier code to be: value = svGetValue (ifcfg, "KEY_MGMT", FALSE); if (!value) goto error; wpa_psk = !strcmp (value, "WPA-PSK"); wpa_eap = !strcmp (value, "WPA-EAP"); ieee8021x = !strcmp (value, "IEEE8021X"); if (!wpa_psk && !wpa_eap && !ieee8021x) goto error; /* Not WPA or Dynamic WEP */ src/settings/nm-settings-connection.c:1816 - coverity[dereference] annotation after g_malloc0(). Shouldn't be needed. src/settings/nm-agent-manager.c:460 - coverity[dereference] annotation after g_malloc0(). Shouldn't be needed. Possibly because the code does "g_assert (req)" after each of the two calls to request_new(), so removing those assertions may fix the coverity warning. src/wifi/wifi-utils.c:41 src/wifi/wifi-utils-nl80211.c:712 - coverity[dereference] annotation after g_malloc0(). Shouldn't be needed. src/dhcp-manager/nm-dhcp-manager.c:299, 302 - DHCLIENT_PATH and DHCPCD_PATH can't be NULL, and coverity was right to warn. We should remove the annotations and the NULL checks. cli/src/utils.c:799 - coverity[dereference] annotation after g_malloc0(). Shouldn't be needed. cli/src/connections.c: tui/nmtui.c: annotations are needed to work around coverity misunderstanding the code
(In reply to comment #4) I agree with reworking the last commit. but the first commits seem uncontroversial and fix real bugs. ACK to push them earlier? > ifcfg-rh: fix leak in svOpenFileInternal() > ifcfg-rh: fix crash for reading invalid bridge configuration > core: fix leaks for nm_setting_ip[46]_config_add_\(route\|address\)() > core: fix potential crash in nm-dhcp-client
yes, those four commits are fine to push now
Branch looks good to me, though the change from bridge -> old_value in ifcfg-rh/reader.c looks superfluous? You'll also run into conflicts when rebasing on top of master before merging in ifcfg-rh/reader.c, FYI, due to the danw/errors2 merge.
For commits merged to master: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=6ba897120cbaf402da0901eafe95130d4e698c9d Remaining commits rebase on master and pushed to th/bg728320_coverity... (In reply to comment #7) > Branch looks good to me, though the change from bridge -> old_value in > ifcfg-rh/reader.c looks superfluous? sorry, say you comment too late... > > You'll also run into conflicts when rebasing on top of master before merging in > ifcfg-rh/reader.c, FYI, due to the danw/errors2 merge. Yepp. I fixed that.
s/sorry, say you comment too late.../sorry, I saw your comment too late.../ :)
(In reply to comment #7) > Branch looks good to me, though the change from bridge -> old_value in > ifcfg-rh/reader.c looks superfluous? It's not; if NMSettingConnection:master is already set at that point, it would be because there's a bond master. So calling that value "bridge" doesn't make sense. (That part of the patch came from me; by a strange coincidence, I happened to notice the two-year-old double-free in that code the day after Thomas did.)
For the moment, I rebased the branch to current master and puhsed (non-ff). The commit is now split in two parts: 9ef2394 all: fix various warnings detected with coverity 9d56426 fixup! all: fix various warnings detected with coverity The first commit should be uncontroversial (please review that one first, and if ACK'ed I merge it earlier). With the second commit, coverity reports no more warnings, but according to comment 4 we might not like this solution. I will rework that part.
I still don't like the tui/nm-editor-utils.c patch, since it seems to be replacing the code with basically completely-identical code, which implies that we're missing something. All the other changes in the first patch are fine though.
(In reply to comment #12) > I still don't like the tui/nm-editor-utils.c patch, since it seems to be > replacing the code with basically completely-identical code, which implies that > we're missing something. All the other changes in the first patch are fine > though. Ok, since I don't know how to fix tui/nm-editor-utils.c otherwise, I accept this. Side note: if we are confident that this condition never arises, an g_assert would be more appropriate. I think we should use g_return* for cases where we are not entirely sure that this cannot happen, but the code should also work if compiled with -DG_DISABLE_CHECKS (at least in principle. I know, that is currently often not the case, but that should be our aim). The patch fixes that.
Created attachment 275657 [details] coverity scan of current master [commit 9ef2394] Pushed commit 9ef2394. Attached a new coverity scan of 9ef2394. http://cov01.lab.eng.brq.redhat.com/covscanhub/task/11566/ (note that current th/bg728320_coverity fixes all those warnings) Btw. note that g_mallo0() actually returns %NULL, if b_bytes==0. I guess, this is the reason for the warning...
(In reply to comment #13) > Side note: if we are confident that this condition never arises, an g_assert > would be more appropriate. > I think we should use g_return* for cases where we are not entirely sure that > this cannot happen, but the code should also work if compiled with > -DG_DISABLE_CHECKS (at least in principle. I know, that is currently often not > the case, but that should be our aim). The patch fixes that. g_return_if* is intended to catch cases where the function's caller has done something wrong. Eg, in the nmt-editor-utils.c case, you're only supposed to call nm_editor_utils_create_connection() with types that are in the list returned from nm_editor_utils_get_connection_type_list(), so if we get to the end of the loop and type_data is still NULL, then the caller messed up. So it shouldn't ever be possible with a g_return_if* to prove locally that the condition never arises. (In this case, since nmt-editor-utils isn't currently part of a library, we could look at all of its call sites in nmtui, and prove that they're using the API correctly. But that doesn't guarantee that it will continue to always be used correctly in the future.) (In reply to comment #14) > Btw. note that g_mallo0() actually returns %NULL, if b_bytes==0. I guess, this > is the reason for the warning... Yeah, I thought that might be the issue at one point, but g_malloc() returns NULL in that case too, and coverity isn't warning us about that. (Although... we don't use g_malloc() as much as g_malloc0(), and in most cases, coverity might be able to prove that the passed-in length is non-0. Though that would be pretty tough to prove in src/settings/plugins/ifcfg-rh/shvar.c:svEscape()...)
I modified the branch and repushed. I don't know, how to appease coverity in any other way. Dan, if you are still unhappy with these changes, please take over and fix them however you like.
(In reply to comment #15) > (In reply to comment #14) > > Btw. note that g_mallo0() actually returns %NULL, if b_bytes==0. I guess, this > > is the reason for the warning... > > Yeah, I thought that might be the issue at one point, but g_malloc() returns > NULL in that case too, and coverity isn't warning us about that. (Although... > we don't use g_malloc() as much as g_malloc0(), and in most cases, coverity > might be able to prove that the passed-in length is non-0. Though that would be > pretty tough to prove in src/settings/plugins/ifcfg-rh/shvar.c:svEscape()...) btw, commit "wifi: change num_freqs type to unsigned" fixes the warning by - info->freqs = g_malloc0 (sizeof (guint32) * info->num_freqs); + info->freqs = g_malloc0 (sizeof (guint32) * MAX (1, info->num_freqs)); but the following pattern seems not to fix the warning: g_assert (n_bytes > 0); ptr = g_malloc0 (n_bytes);
After trying some more and failing to get coverity to do the right thing, I filed https://engineering.redhat.com/trac/CoverityScan/ticket/87 to get a "model" added to covscan to recognize that g_malloc0 does not return NULL.
(In reply to comment #17) > btw, commit "wifi: change num_freqs type to unsigned" fixes the warning by > > - info->freqs = g_malloc0 (sizeof (guint32) * info->num_freqs); > + info->freqs = g_malloc0 (sizeof (guint32) * MAX (1, info->num_freqs)); > > but the following pattern seems not to fix the warning: > > g_assert (n_bytes > 0); > ptr = g_malloc0 (n_bytes); So, apparently g_malloc0() already is modeled in covscan, as only ever returning NULL if the passed-in size is 0. So the warnings we're getting *are* because it thinks we might possibly be malloc'ing 0 bytes. So the problem seems to be maybe that it doesn't recognize g_assert() as being an assertion?
This bug is old and obsoleted by fixes in bug 741122.