| Message ID | 20190709221312.7089-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IB/rdmavt: Remove err declaration in if statement in rvt_create_cq | expand |
On Tue, Jul 9, 2019 at 3:13 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > clang warns: > > drivers/infiniband/sw/rdmavt/cq.c:260:7: warning: variable 'err' is used Oh, !$*@, this is a tricky one. While the if scoped `err` declared on L250 is initialized when used at L260, the function scoped `err` declared on L211 is not initialized when it is used on L310 when control flow enters the if on L249 then the goto on L255 or L261. So this is a bug due to the if scoped `err` "shadowing" the function scoped `err`. Maybe not important enough to send a v2, but I feel like the commit message should say something along the lines of `fix a potential use of uninitialized memory due to shadowing`. Either way, this fixes a real bug, so thanks for the patch. Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > uninitialized whenever 'if' condition is true > [-Wsometimes-uninitialized] > if (err) > ^~~ > drivers/infiniband/sw/rdmavt/cq.c:310:9: note: uninitialized use occurs > here > return err; > ^~~ > drivers/infiniband/sw/rdmavt/cq.c:260:3: note: remove the 'if' if its > condition is always false > if (err) > ^~~~~~~~ > drivers/infiniband/sw/rdmavt/cq.c:253:7: warning: variable 'err' is used > uninitialized whenever 'if' condition is true > [-Wsometimes-uninitialized] > if (!cq->ip) { > ^~~~~~~ > drivers/infiniband/sw/rdmavt/cq.c:310:9: note: uninitialized use occurs > here > return err; > ^~~ > drivers/infiniband/sw/rdmavt/cq.c:253:3: note: remove the 'if' if its > condition is always false > if (!cq->ip) { > ^~~~~~~~~~~~~~ > drivers/infiniband/sw/rdmavt/cq.c:211:9: note: initialize the variable > 'err' to silence this warning > int err; > ^ > = 0 > 2 warnings generated. > > There are two err declarations in this function: at the top and within > an if statement; clang warns because the assignments to err in the if > statement are local to the if statement so err will be used > uninitialized if this error handling is used. Remove the if statement's > err declaration so that everything works properly. > > Fixes: 239b0e52d8aa ("IB/hfi1: Move rvt_cq_wc struct into uapi directory") > Link: https://github.com/ClangBuiltLinux/linux/issues/594 > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > drivers/infiniband/sw/rdmavt/cq.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/infiniband/sw/rdmavt/cq.c b/drivers/infiniband/sw/rdmavt/cq.c > index fac87b13329d..a85571a4cf57 100644 > --- a/drivers/infiniband/sw/rdmavt/cq.c > +++ b/drivers/infiniband/sw/rdmavt/cq.c > @@ -247,8 +247,6 @@ int rvt_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr, > * See rvt_mmap() for details. > */ > if (udata && udata->outlen >= sizeof(__u64)) { > - int err; > -
On Tue, Jul 09, 2019 at 03:38:57PM -0700, Nick Desaulniers wrote: > On Tue, Jul 9, 2019 at 3:13 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > clang warns: > > > > drivers/infiniband/sw/rdmavt/cq.c:260:7: warning: variable 'err' is used > > Oh, !$*@, this is a tricky one. While the if scoped `err` declared on > L250 is initialized when used at L260, the function scoped `err` > declared on L211 is not initialized when it is used on L310 when > control flow enters the if on L249 then the goto on L255 or L261. So > this is a bug due to the if scoped `err` "shadowing" the function > scoped `err`. > > Maybe not important enough to send a v2, but I feel like the commit > message should say something along the lines of `fix a potential use > of uninitialized memory due to shadowing`. Either way, this fixes a > real bug, so thanks for the patch. > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Gah, shadowing was the word I was looking for. Knew the behavior, didn't remember what it was called. I like the clarity of your explanation better so I will clean up the message and send a v2 shortly with your reviewed by. Thanks for the review! Nathan
diff --git a/drivers/infiniband/sw/rdmavt/cq.c b/drivers/infiniband/sw/rdmavt/cq.c index fac87b13329d..a85571a4cf57 100644 --- a/drivers/infiniband/sw/rdmavt/cq.c +++ b/drivers/infiniband/sw/rdmavt/cq.c @@ -247,8 +247,6 @@ int rvt_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr, * See rvt_mmap() for details. */ if (udata && udata->outlen >= sizeof(__u64)) { - int err; - cq->ip = rvt_create_mmap_info(rdi, sz, udata, u_wc); if (!cq->ip) { err = -ENOMEM;
clang warns: drivers/infiniband/sw/rdmavt/cq.c:260:7: warning: variable 'err' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] if (err) ^~~ drivers/infiniband/sw/rdmavt/cq.c:310:9: note: uninitialized use occurs here return err; ^~~ drivers/infiniband/sw/rdmavt/cq.c:260:3: note: remove the 'if' if its condition is always false if (err) ^~~~~~~~ drivers/infiniband/sw/rdmavt/cq.c:253:7: warning: variable 'err' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] if (!cq->ip) { ^~~~~~~ drivers/infiniband/sw/rdmavt/cq.c:310:9: note: uninitialized use occurs here return err; ^~~ drivers/infiniband/sw/rdmavt/cq.c:253:3: note: remove the 'if' if its condition is always false if (!cq->ip) { ^~~~~~~~~~~~~~ drivers/infiniband/sw/rdmavt/cq.c:211:9: note: initialize the variable 'err' to silence this warning int err; ^ = 0 2 warnings generated. There are two err declarations in this function: at the top and within an if statement; clang warns because the assignments to err in the if statement are local to the if statement so err will be used uninitialized if this error handling is used. Remove the if statement's err declaration so that everything works properly. Fixes: 239b0e52d8aa ("IB/hfi1: Move rvt_cq_wc struct into uapi directory") Link: https://github.com/ClangBuiltLinux/linux/issues/594 Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- drivers/infiniband/sw/rdmavt/cq.c | 2 -- 1 file changed, 2 deletions(-)