Fixes: #4673 dht: fix wrong free of borrowed pointer in init error path#4672
Open
ThalesBarretto wants to merge 1 commit into
Open
Fixes: #4673 dht: fix wrong free of borrowed pointer in init error path#4672ThalesBarretto wants to merge 1 commit into
ThalesBarretto wants to merge 1 commit into
Conversation
GF_OPTION_INIT("xattr-name", conf->xattr_name, str, err) stores a
borrowed pointer via pass() — either opt->default_value (.rodata) or
data->data from dict_get_str() (dict-internal). Neither is owned by
the caller. The err: block at dht-shared.c:841 calls GF_FREE on it,
which causes __gf_free to read a fake mem_header from unrelated memory
and corrupt heap state.
dht_fini() correctly omits this free. The derived names (link_xattr_name,
wild_xattr_name, mds_xattr_key, commithash_xattr_name) are owned via
gf_asprintf and correctly freed.
Introduced in 76bc5d1 (2013) when xattr-name configurability was
added for tiering. The error block freed all names uniformly, not
realizing GF_OPTION_INIT str borrows rather than allocates.
Signed-off-by: Thales Antunes de Oliveira Barretto <[email protected]>
# xlators/cluster/dht/src/dht-shared.c | 1 -
# 1 file changed, 1 deletion(-)
ThalesBarretto
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #4673
dht_init()error path callsGF_FREE(conf->xattr_name)on a pointer that wasnever heap-allocated.
GF_OPTION_INIT("xattr-name", conf->xattr_name, str, err)stores a borrowed pointer via the
pass()conversion function(
libglusterfs/src/options.c:1314: *out = in) — it points either toopt->default_value(a.rodatastring literal"trusted.glusterfs.dht") or todict-internal storage from
dict_get_str(). Neither is caller-owned.When
__gf_free()is called on this pointer, it subtractsGF_MEM_HEADER_SIZEand interprets the preceding memory as a
struct mem_header— reading garbage.The
GF_ASSERTon the magic field fires but continues (SIGCONT), leading to heapmetadata corruption.
dht_fini()correctly does not free this pointer. The four derived xattr names(
link_xattr_name,wild_xattr_name,mds_xattr_key,commithash_xattr_name)are allocated via
gf_asprintfand correctly freed in both paths — the asymmetrybetween the borrowed base name and the owned derived names is what caused the bug.
The line was introduced in 76bc5d1 (2013, "dht: make DHT xattr names
configurable") when the
xattr-nameoption was added for a tiering vision. Thatcommit added
conf->xattr_namealongside twogf_asprintf'd derived names andfreed all three uniformly in the error block — not realizing
GF_OPTION_INIT strreturns a borrowed pointer while
gf_asprintfreturns an owned one. The tieringfeature was later removed (2019–2020) but the option itself cannot be deleted
because
dht_initcallsGF_OPTION_INIT("xattr-name", ...)unconditionally —removing it from
dht_options[]would break volume startup.Fix: delete the wrong
GF_FREE. One-line change, no functional impact.