Messages in this thread Patch in this message | | | From | Arnd Bergmann <> | Date | Thu, 21 Mar 2019 12:45:02 +0100 | 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@googlegroups.com> wrote: > On Wed, Mar 20, 2019 at 12:07 PM Nathan Chancellor > <natechancellor@gmail.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
| |