lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 21 Mar 2019 12:45:02 +0100 From: Arnd Bergmann <arnd@...db.de> To: Nick Desaulniers <ndesaulniers@...gle.com> Cc: Nathan Chancellor <natechancellor@...il.com>, Jon Maloy <jon.maloy@...csson.com>, Ying Xue <ying.xue@...driver.com>, "David S. Miller" <davem@...emloft.net>, tipc-discussion@...ts.sourceforge.net, Networking <netdev@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>, clang-built-linux@...glegroups.com Subject: Re: -Wsometimes-uninitialized Clang warning in net/tipc/node.c On Wed, Mar 20, 2019 at 9:51 PM 'Nick Desaulniers' via Clang Built Linux <clang-built-linux@...glegroups.com> wrote: > On Wed, Mar 20, 2019 at 12:07 PM Nathan Chancellor > <natechancellor@...il.com> wrote: > > The use in tipc_bearer_xmit() isn't even guaranteed to set the in/out > parameter, so even if the if is taken doesn't guarantee that maddr is > always initialized before calling tipc_bearer_xmit(). Right, it is only initialized in certain states. It was always initialized until commit 598411d70f85 ("tipc: make resetting of links non-atomic"), afterwards only if the link was not reset, and as of commit 73f646cec354 ("tipc: delay ESTABLISH state event when link is established") only if it's not 'establishing' or 'reset'. > At the minimum, we should initialize maddr to NULL. I think we'd > prefer to risk the possibility of a null pointer dereference to the > possibility of working with uninitialized memory. To be clear, both > are bad, but one is easier to spot/debug later than the other. I disagree with setting it to NULL, given that it is still an obviously incorrect value. We could add a if(maddr) check before calling tipc_bearer_xmit(), but I think it would be clearer to check skb_queue_empty(xmitq)) if that avoids the warning: diff --git a/net/tipc/node.c b/net/tipc/node.c index 2dc4919ab23c..147786795e48 100644 --- a/net/tipc/node.c +++ b/net/tipc/node.c @@ -844,7 +844,8 @@ static void tipc_node_link_down(struct tipc_node *n, int bearer_id, bool delete) tipc_node_write_unlock(n); if (delete) tipc_mon_remove_peer(n->net, n->addr, old_bearer_id); - tipc_bearer_xmit(n->net, bearer_id, &xmitq, maddr); + if (skb_queue_empty(xmitq)) + tipc_bearer_xmit(n->net, bearer_id, &xmitq, maddr); tipc_sk_rcv(n->net, &le->inputq); } This duplicates the check inside of skb_queue_empty(), but I don't know if the compiler can see through the logic behind the inlined function calls. Arnd
Powered by blists - more mailing lists