HDFGroup / HDFGroup/vol-daos

H5_daos_term() can leak the H5_DAOS_G_INIT() connector-ID reference outside full library shutdown

Open
#74 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
5
Forks
11
PR merge metrics
No merged PRs in 30d

Description

## Background

Follow-up from review discussion on #71 (https://github.com/HDFGroup/vol-daos/pull/71#discussion_r3541913478).

\`H5_DAOS_G_INIT()\` (src/daos_vol_private.h) calls \`H5VLget_connector_id_by_value()\` to ensure \`H5_DAOS_g\` is set. Per the HDF5 docs, this is a "get" call that returns a new reference and should be released with \`H5VLclose()\` once no longer needed - unlike the older "peek" form, which returned the ID without taking a reference.

\`H5_daos_term()\` (src/daos_vol.c) intentionally does *not* release this reference. It was tried (244ea63) and reverted (2596e9d) because \`H5_daos_term()\` is itself invoked while closing the last reference to that same ID, so decrementing it again from inside the callback recurses into still-being-torn-down library state - this was reproduced as a 100%-reproducible DAOS task scheduler assertion abort (\`tse_sched_progress: Assertion '0' failed\`) on process shutdown in CI.

## The gap

Per @jhendersonHDF:

> Though it's very much an edge case, this is still a leak and needs to be closed in some cases to be clean. The termination function could be called outside of just the library terminating, which would leak the ID and also leave an incremented reference count on the associated connector object structure. [...] \`H5is_library_terminating()\` might be usable here, though that does seem like hacking around design issues and it also still has to be known whether \`H5_DAOS_g\` was actually initialized by \`H5VLget_connector_id_by_value()\` or somewhere else.

So a correct fix needs two things neither of which exist today:
1. A way to distinguish "H5_daos_term() fired because the library is doing a forced full shutdown" (safe to leave the reference - HDF5 tears the atom down regardless of refcount) from "H5_daos_term() fired via a normal refcounted close reaching zero outside of shutdown" (the reference should actually be released, and it's safe to do so since we're not racing our own atom's forced teardown) - candidate: \`H5is_library_terminating()\`, with the caveat that it may itself be hacking around a design issue in the VOL terminate-callback contract.
2. Provenance tracking for \`H5_DAOS_g\`: it can currently be set via \`H5_DAOS_G_INIT()\`'s \`H5VLget_connector_id_by_value()\` call, or via \`H5Pset_fapl_daos()\`'s \`H5VLregister_connector()\` / \`H5VLget_connector_id_by_name()\` paths, which have different ownership semantics. Only the \`H5_DAOS_G_INIT()\` case is the one this issue is about.

## Why not fixed now

Confirmed low priority by @jhendersonHDF ("Probably not worth worrying about right now, but it is a design problem that could cause issues"), and a wrong attempt here has already caused a real, 100%-reproducible crash in CI. Filing this so the reasoning and the two missing pieces aren't lost.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with H5_DAOS_G_INIT() in src/daos_vol_private.h and H5_daos_term() in src/daos_vol.c, then trace the H5VLget_connector_id_by_value(), H5VLregister_connector(), and H5VLget_connector_id_by_name() paths. Review commits 244ea63 and 2596e9d and the shutdown behavior before defining a fix that releases only the reference owned by H5_DAOS_G_INIT() without triggering the shutdown assertion.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
backend
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.