Skip to content

DAOS-18304 ddb: Fix memory leaks, defer ordering, and pool handle corruption#18125

Merged
daltonbohning merged 5 commits into
masterfrom
ckochhof/fix/master/daos-18304-patch-000
May 6, 2026
Merged

DAOS-18304 ddb: Fix memory leaks, defer ordering, and pool handle corruption#18125
daltonbohning merged 5 commits into
masterfrom
ckochhof/fix/master/daos-18304-patch-000

Conversation

@knard38
Copy link
Copy Markdown
Contributor

@knard38 knard38 commented Apr 28, 2026

Description

This PR is the first split of #17967, broken out at reviewers' request to make the review process easier. It contains unrelated code fixes found during the creation of the initial PR. The second split #18124 contains the main refactoring and will be reviewed separately.

Go layer

  • Feature(): C.CString(enable) and C.CString(disable) were passed directly to ddb_feature_string2flags() without ever being freed; assign them to named variables so defer freeString() can release them.
  • DtxStat(): options.details was set before defer freeString(options.path), inverting the defer execution order; move the defer immediately after the CString allocation.

C layer

  • ddb_run_feature(): dc_poh and dc_write_mode were reset unconditionally, corrupting the caller's pool state when the pool was already open before the call; guard both resets inside the if (close) block.
  • helper_stat_open_modify_close_stat(): the function overwrote tctx->dvt_poh with a temporary handle and never restored it, leaving the suite-level handle invalid for subsequent tests; save and restore the original handle around the open/close sequence.
  • read_only_vs_write_mode_test(): assert_memory_equal() used fs[FILE_STATE_PRE].digest as both arguments, making the read-only assertion a no-op; use fs[FILE_STATE_POST].digest as the second argument.

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).

When the pool was already open before ddb_run_feature() was called (i.e.
the 'close' flag is false), resetting dc_poh to DAOS_HDL_INVAL and
dc_write_mode to false unconditionally corrupted the caller's pool state.
Guard these resets inside the 'if (close)' block so they only execute
when this function itself opened the pool.

Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
- Feature(): C.CString(enable) and C.CString(disable) were passed
  directly to ddb_feature_string2flags() without ever being freed.
  Assign them to named variables so defer freeString() can release them.

- DtxStat(): options.details was set before defer freeString(options.path),
  inverting the defer execution order. Move the defer immediately after
  the CString allocation.

Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
- helper_stat_open_modify_close_stat() overwrote tctx->dvt_poh with the
  temporary pool handle opened for the test and never restored it, leaving
  the suite-level pool handle invalid for all subsequent tests.
  Save and restore the original handle around the open/close sequence.

- read_only_vs_write_mode_test() passed fs[FILE_STATE_PRE].digest as both
  arguments to assert_memory_equal(), comparing the pre-digest against
  itself and making the read-only assertion a no-op. Use
  fs[FILE_STATE_POST].digest as the second argument.

Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
@knard38 knard38 changed the title DAOS-18304 ddb: TODO DAOS-18304 ddb: Fix memory leaks, defer ordering, and pool handle corruption Apr 28, 2026
@knard38 knard38 self-assigned this Apr 28, 2026
@knard38 knard38 marked this pull request as ready for review April 28, 2026 15:37
@knard38 knard38 requested review from a team as code owners April 28, 2026 15:37
@github-actions
Copy link
Copy Markdown

Ticket title is 'Add unit test to ddb go code'
Status is 'In Review'
https://daosio.atlassian.net/browse/DAOS-18304

Fix clang format issue detected by CI.

Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Copy link
Copy Markdown
Contributor

@Nasf-Fan Nasf-Fan left a comment

Choose a reason for hiding this comment

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

Good catching them!

@knard38 knard38 requested a review from a team May 6, 2026 06:25
@knard38
Copy link
Copy Markdown
Contributor Author

knard38 commented May 6, 2026

@daos-stack/daos-gatekeeper could you lend this PR. The only pending CI step has been skipped by the CI itself. If possible, I would like to avoid to restart a CI round as it is very unstable lately. If it is OK, could you lend it with this message:

  • title: DAOS-18304 ddb: Fix memory leaks, defer ordering, and pool handle corruption
  • message:
   Go layer:
   - Feature(): assign C.CString(enable/disable) to named variables so
     defer freeString() can release them; previously both CStrings were
     passed directly to ddb_feature_string2flags() and never freed.
   - DtxStat(): move defer freeString(options.path) immediately after the
     CString allocation, before options.details is set, to restore correct
     defer execution order.

   C layer:
   - ddb_run_feature(): guard dc_poh and dc_write_mode resets inside the
     if (close) block to avoid corrupting the caller's pool state when the
     pool was already open before the call.
   - helper_stat_open_modify_close_stat(): save and restore tctx->dvt_poh
     around the open/close sequence so the suite-level handle remains valid
     for subsequent tests.
   - read_only_vs_write_mode_test(): fix assert_memory_equal() that used
     fs[FILE_STATE_PRE].digest as both arguments, making the read-only
     assertion a no-op; use fs[FILE_STATE_POST].digest as second argument.

Copy link
Copy Markdown
Contributor

@tanabarr tanabarr left a comment

Choose a reason for hiding this comment

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

go changes LGTM

@daltonbohning daltonbohning merged commit ed111d4 into master May 6, 2026
44 checks passed
@daltonbohning daltonbohning deleted the ckochhof/fix/master/daos-18304-patch-000 branch May 6, 2026 13:30
grom72 pushed a commit that referenced this pull request May 11, 2026
…ruption (#18125)

Go layer:
- Feature(): assign C.CString(enable/disable) to named variables so
  defer freeString() can release them; previously both CStrings were
  passed directly to ddb_feature_string2flags() and never freed.
- DtxStat(): move defer freeString(options.path) immediately after the
  CString allocation, before options.details is set, to restore correct
  defer execution order.

C layer:
- ddb_run_feature(): guard dc_poh and dc_write_mode resets inside the
  if (close) block to avoid corrupting the caller's pool state when the
  pool was already open before the call.
- helper_stat_open_modify_close_stat(): save and restore tctx->dvt_poh
  around the open/close sequence so the suite-level handle remains valid
  for subsequent tests.
- read_only_vs_write_mode_test(): fix assert_memory_equal() that used
  fs[FILE_STATE_PRE].digest as both arguments, making the read-only
  assertion a no-op; use fs[FILE_STATE_POST].digest as second argument.

Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
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.

6 participants