field-cache: define string ownership - #136
Merged
Merged
Conversation
DC_FIELD_STRING callers have long owned the returned value, but the field cache returned an alias of its own allocation. Freeing cached strings during parser destruction consequently double-freed values already released by consumers. Return a duplicate to callers, free cache-owned originals when cache-backed parsers are reset or destroyed, and release strings in dctool after writing them. Document the caller-owned API contract. Signed-off-by: Michael Keller <github@ike.ch>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The only remaining feedback is a minor documentation nit, not an approval blocker.
Pull request overview
Defines ownership for cached DC_FIELD_STRING values to prevent double frees and leaks.
Changes:
- Return caller-owned string duplicates.
- Clean up cache allocations during parser reset/destruction.
- Document ownership and free returned values in the XML example.
File summaries
| File | Changes |
|---|---|
src/field-cache.c |
Duplicates cached strings and cleans up cache allocations. |
include/libdivecomputer/parser.h |
Documents caller-owned values; clarify that only value must be freed. |
examples/output_xml.c |
Frees returned string values after use. |
Review details
Suppressed comments (1)
include/libdivecomputer/parser.h:365
dc_field_string_thas bothdescandvalue, but onlyvalueis duplicated for the caller;descremains a static/cache-owned pointer (src/field-cache.c:11-16, 61-62). This wording can lead callers to freestr.descas well and trigger an invalid free. Please state explicitly that onlydc_field_string_t.valuemust be freed.
/* DC_FIELD_STRING values are caller-owned and must be freed after use. */
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
DC_FIELD_STRING callers have long owned the returned value, but the field cache returned an alias of its own allocation. Freeing cached strings during parser destruction consequently double-freed values already released by consumers.
Return a duplicate to callers, free cache-owned originals when cache-backed parsers are reset or destroyed, and release strings in dctool after writing them. Document the caller-owned API contract.
Fixes a crash introduced in #130.