On 2/25/2019 10:26 PM, Shaobo He wrote: > Hello everyone, > > I'd like to point out two issues about the function `post_terminate`. > First of all, this function is called only once at line 2069. In its > only call site, the second argument is NULL and the `ep` field of the > first argument could be NULL if the `err` block is executed. There are > two possible issues then. > > First, there is practically dead code in `build_term_codes` because it > is only called by `post_terminate` with the first argument being NULL. > This means, in practical, only the code in the first `if` statement can > be possibly executed which only composes a small fraction of > `build_term_codes`'s body. > > Second, there may be NULL pointer dereference in `post_terminate` > because in case `qhp->attr.state` is `C4IW_QP_STATE_RTS` and > `attrs->next_state` is `C4IW_QP_STATE_TERMINATE`, `terminate` may be set > to 1 when `internal` is not 0, which leads to `qhp->ep` set to NULL and > further NULL pointer dereference in `post_terminate`. > > Please let me know if it makes sense or not. > Hey Shaobo, Yes, these look like legitimate issues. If the err block is branched to, then the local variable terminate should be set to 0. IE if the QP is transitioned to ERROR then no TERMINATE should be ever be sent. And build_term_codes() is indeed pretty useless since nobody provides the error cqe. To make it useful, callers of c4iw_modify_qp() with internal == 1, should provide an error cqe if they have one. In particular, post_qp_event() could provide the error cqe. Actually, struct c4iw_qp_attributes has layer_etype and ecode fields. I think the intention was to fill these out. So that is one path, or we could replace those fields with a err_cqe pointer and post_qp_event() could provide the entire cqe so build_term_codes() could work as is and remain static. Steve.
