Coverity CID 1700533 - Handle fr_hash_table_delete return

[ethan-thompson]

Possible returns for fr_hash_table_delete and how we handle them (and reasoning):

• -1 means the comparator errored mid-search, so file may still be linked into the table. This really shouldn't happen, especially since it is just a strcmp, but if it does happen then this is fatal since there is no safe recovery available here to prevent a potential use-after-free.
• 0 the delete was fine. We don't need to do anything else.
• 1 the entry was not found, meaning file wasn't in the file_table. This shouldn't happen given the insert always precedes talloc_set_destructor(), and nothing is dangling, so we are safe to free. We log it (and panic in debug builds) even though nothing acts on the result, since we don't want to silently miss this case in a future code change.

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[arr2036]

@claude review

[claude]

Claude finished @arr2036's task in 2m 48s —— View job

---

Claude Code Review

• Gather context (repo, PR diff)
• Analyze fr_hash_table_delete / comparator semantics
• Review the change
• Post review

---

Reviewed src/modules/rlm_linelog/file.c (+9/-1). The tri-state handling matches the documented contract of fr_hash_table_delete() (src/lib/util/hash.c:625-646), and the fr_cond_assert_msg(...) → return -1 shape mirrors the existing idiom at src/lib/util/event.c:839-843. Three things worth considering, plus one nit.

---

1. Returning -1 from a talloc destructor is not a safe no-op during teardown (medium)

src/modules/rlm_linelog/file.c:104

_file_free() is installed via talloc_set_destructor() (file.c:324), and file is a child of mctx->thread (file.c:298). There is no explicit thread-detach that frees the files — they are reaped when the thread ctx is freed.

When talloc frees a parent and a child's destructor returns -1, talloc does not abort the parent free. _talloc_free_children_internal() steals the surviving child up to the grandparent and carries on freeing the parent. So on module thread teardown you'd get:

• thread (and with it thread->file_table and thread->tl) freed anyway,
• file reparented and still alive, with file->thread_inst now dangling,
• file->write / file->expiry timers still holding file->thread_inst->tl,
• and _file_free() running a second time when the grandparent is freed — dereferencing file->thread_inst->file_table on freed memory.

That trades Coverity's "unchecked return / possibly-stale table entry" for a potential use-after-free. There isn't really a recovery path here — if the entry can't be unlinked there's nothing sensible to do with file — so fr_fatal_assert_msg() (src/lib/util/debug.h:217) arguably expresses the intent more honestly than refusing the free. At minimum the comment on line 104 is worth expanding, since "leave the entry in the table" only holds while the table is still alive.

Fix this →

2. The comparator's fr_strerror() is never consumed (minor)

src/modules/rlm_linelog/file.c:103

src/lib/util/misc.h:42-44 states that a comparator returning CMP_ERR "also pushes a fr_strerror describing the operand problem", and fr_hash_table_delete()'s docs say to "retrieve the error with fr_strerror". The assert message reports only the filename, so the actual cause is lost, and the unconsumed entry can leak into a later, unrelated error report. The event.c idiom this follows does include the cause (fr_syserror(errno) at event.c:841).

if (!fr_cond_assert_msg(ret >= 0, "Failed removing \"%s\" from file_table: %s",
                        file->filename, fr_strerror())) {

Fix this →

3. The PR description understates what fr_cond_assert_msg does on the ret == 1 path

src/modules/rlm_linelog/file.c:107

The description says the not-found case is one where "we log it even though nothing acts on the result". In any build without NDEBUG, fr_cond_assert_msg() → _fr_assert_fail() calls fr_fault(SIGABRT) (src/lib/util/debug.c:1282-1284) — it aborts, it doesn't just log. That's a defensible choice for a should-never-happen invariant, but "panic if in debug" applies to both branches, not just -1. Worth correcting in the commit message so the intent is clear to whoever reads it next.

4. Note: the -1 branch is currently unreachable

filename_cmp() (file.c:38-44) is CMP(strcmp(...), 0), which yields only CMP_LT/CMP_EQ/CMP_GT — never CMP_ERR (INT8_MIN, misc.h:51). list_find() only returns -1 on CMP_ERR (hash.c:197). So this is purely future-proofing, which is fine and matches the stated rationale — it just means the -1 handling should be judged on whether it's safe, not on whether it fires (hence point 1).

5. Nit

file.c:99-101 could collapse to int ret = fr_hash_table_delete(file->thread_inst->file_table, file);.

---

Things that check out

• The hash table is created with a NULL free function (file.c:370), so fr_hash_table_delete()'s ht->free(old) path (hash.c:643) is inert — no double-free risk from the destructor calling delete.
• file->filename is a talloc child of file and is still valid inside the destructor, so it's safe to use in the assert messages.
• The talloc_free(file) error path at file.c:309 runs before talloc_set_destructor() at file.c:324, so it can't trip either assert.
• file_table is allocated before any file (file_thread_init()), so talloc's LIFO child teardown frees the files first — the table is still live when the destructors run.

I did not build or run the test suite for this review.
· fix/coverity-linelog-file-free

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[ethan-thompson]

Addressing the Claude review:

1. Addressed by using fr_fatal_assert_msg since there is no safe recovery available here
2. Addressed by using fr_strerror in the fatal message
3. Updated to accurately reflect the behaviour
4. Correct. But this change prevents a future change that makes the branch reachable from causing more problems
5. Disagree on format unless the maintainers say otherwise

[arr2036]

Could you rebase and push again, if nothing else it's useful to check cI :)