On Tue, Jan 09, 2018 at 08:41:09AM -0000, Talat Batheesh wrote:
> Reading the report, there are a lot FALSE positives. For example, all
> resp.* warnings are false. resp is initialize to be zero.
Hello Talat, at least in the version I'm reviewing, this does not appear
to be the case.
Consider providers/mlx4/mlx4.c mlx4_init_context():
:g/resp/p
| struct mlx4_alloc_ucontext_resp resp;
| struct mlx4_alloc_ucontext_resp_v3 resp_v3;
| &resp_v3.ibv_resp, sizeof resp_v3))
| context->num_qps = resp_v3.qp_tab_size;
| bf_reg_size = resp_v3.bf_reg_size;
| &resp.ibv_resp, sizeof resp))
| context->num_qps = resp.qp_tab_size;
| bf_reg_size = resp.bf_reg_size;
| if (resp.dev_caps & MLX4_USER_DEV_CAP_64B_CQE)
| context->cqe_size = resp.cqe_size;
None of these lines initalize resp.
The two lines that _might_ have initialized resp and resp_v3 are in
their full:
| if (ibv_cmd_get_context(ibv_ctx, &cmd, sizeof cmd,
| &resp_v3.ibv_resp, sizeof resp_v3))
| [...]
| if (ibv_cmd_get_context(ibv_ctx, &cmd, sizeof cmd,
| &resp.ibv_resp, sizeof resp))
These lines are notable because they're passing the size (sizeof resp)
to a function that was given a pointer to a member inside the struct.
If these structures are ever re-ordered for any reason it's an open
invitation to memory corruption, overflows, etc. (The function doesn't
actually use the size to bound anything -- just as part of constructing
a command.)
Probably every programmer who would work on these tools knows these
structures cannot be re-ordered. But this contributes to what I called
"inconsistent code quality" earlier -- this API is brittle in the face
of changes and long-term maintainence.
| int ibv_cmd_get_context(struct ibv_context *context, struct ibv_get_context
*cmd,
| size_t cmd_size, struct ibv_get_context_resp *resp,
| size_t resp_size)
| {
| if (abi_ver < IB_USER_VERBS_MIN_ABI_VERSION)
| return ENOSYS;
|
| IBV_INIT_CMD_RESP(cmd, cmd_size, GET_CONTEXT, resp, resp_size);
|
| if (write(context->cmd_fd, cmd, cmd_size) != cmd_size)
| return errno;
|
| (void) VALGRIND_MAKE_MEM_DEFINED(resp, resp_size);
|
| context->async_fd = resp->async_fd;
| context->num_comp_vectors = resp->num_comp_vectors;
|
| return 0;
| }
resp is not cleared in this function.
| #define IBV_INIT_CMD_RESP(cmd, size, opcode, out, outsize) \
| do { \
| if (abi_ver > 2) \
| (cmd)->command = IB_USER_VERBS_CMD_##opcode; \
| else \
| (cmd)->command = IB_USER_VERBS_CMD_##opcode##_V2; \
| (cmd)->in_words = (size) / 4; \
| (cmd)->out_words = (outsize) / 4; \
| (cmd)->response = (uintptr_t) (out); \
| } while (0)
resp (aka 'out') is not cleared in this macro.
| static inline void VALGRIND_MAKE_MEM_DEFINED(const void *mem,size_t len) {}
| #define VALGRIND_MAKE_MEM_DEFINED VALGRIND_MAKE_MEM_DEFINED
resp (aka 'mem') is not cleared in this function.
Thus I believe cppcheck's conclusion that resp members are being
used without having been initialized.
Thanks
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1732892
Title:
[MIR] 18.04 rdma-core as replacement for older ibverbs code
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/rdma-core/+bug/1732892/+subscriptions
--
ubuntu-bugs mailing list
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
