Skip to content

DAOS-18681 rebuild: fix potential memory leaks in error paths#17692

Open
wangshilong wants to merge 2 commits intomasterfrom
shilongw/DAOS-18681
Open

DAOS-18681 rebuild: fix potential memory leaks in error paths#17692
wangshilong wants to merge 2 commits intomasterfrom
shilongw/DAOS-18681

Conversation

@wangshilong
Copy link
Contributor

Fix potential memory leaks in error cases in rebuild and IV path. Also Fixed a memmory allocation check typo to avoid potential crash

Removed stale comments

Steps for the author:

  • Commit message follows the guidelines.
  • Appropriate Features or Test-tag pragmas were used.
  • Appropriate Functional Test Stages were run.
  • At least two positive code reviews including at least one code owner from each category referenced in the PR.
  • Testing is complete. If necessary, forced-landing label added and a reason added in a comment.

After all prior steps are complete:

  • Gatekeeper requested (daos-gatekeeper added as a reviewer).

Fix potential memory leaks in error cases in rebuild and IV path.
Also Fixed a memmory allocation check typo to avoid potential crash

Removed stale comments

Signed-off-by: Wang Shilong <shilong.wang@hpe.com>
@wangshilong wangshilong requested review from a team as code owners March 12, 2026 01:19
@wangshilong wangshilong requested review from NiuYawei and liuxuezhao and removed request for a team March 12, 2026 01:19
@github-actions
Copy link

Ticket title is 'Fix potential memory Leaks '
Status is 'Open'
https://daosio.atlassian.net/browse/DAOS-18681

@wangshilong wangshilong requested a review from Nasf-Fan March 12, 2026 01:20
if (local_bulk != CRT_BULK_NULL)
crt_bulk_free(local_bulk);
if (rpc != NULL)
RPC_PUB_DECREF(rpc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After calling crt_req_send(), the RPC reference will be automatically released even if crt_req_send() return failure? So we need to distinguish that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, we need diff that case!

if (local_bulk != CRT_BULK_NULL)
crt_bulk_free(local_bulk);
if (rpc != NULL)
RPC_PUB_DECREF(rpc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.

DL_ERROR(rc, DF_RB " scan broadcast send failed.", DP_RB_RGT(rgt));
crt_req_decref(rpc);
D_GOTO(out, rc);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not for fixing leak, but for skipping "rgt_init_scan = 1' when dss_rpc_send() failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, I think I need revert this change. because even RPC failed, it might already send some RPCs to some targets, init_scan is used to check if we need kick off FAIL_RECLAIM task if rebuild fail for some reasons, if RPC has already reached some nodes(migration might have migrated some data, by design FAIL reclaim should be triggered in this case). So probably this is what original author wrote on purpose(even it is a bit strange and easy to trigger AI warnings)

Signed-off-by: Wang Shilong <shilong.wang@hpe.com>
@liuxuezhao
Copy link
Contributor

good finding, is it cached by AI or by reading code?

@wangshilong
Copy link
Contributor Author

good finding, is it cached by AI or by reading code?

It is by AI.

@daosbuild3
Copy link
Collaborator

Test stage Functional Hardware Medium MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-17692/3/execution/node/429/log

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants