Skip to content

Introduce __attribute__((cleanup)) for memory management. - #20

Open
kivkiv12345 wants to merge 1 commit into
masterfrom
__attribute__((cleanup))
Open

Introduce __attribute__((cleanup)) for memory management.#20
kivkiv12345 wants to merge 1 commit into
masterfrom
__attribute__((cleanup))

Conversation

@kivkiv12345

Copy link
Copy Markdown
Contributor

This commit makes us more dependant on GCC,
but introduces a more reliable way to free optparse_t pointers.

Currently this pull request solves the following memory leaks:

2 missing calls to optparse_del() in loki.c (loki_start_cmd())
6 missing calls to optparse_del() in slash_apm.c (apm_load_cmd(), apm_info_cmd())
1 call to optparse_del() after return in prometheus.c (prometheus_start_cmd())

1 missing call to optparse_del() in builtins.c (slash_builtin_watch())

csp_init_cmd() has been spared, as we deliberately leak "parser" there to keep some string pointers.

This commit makes us more dependant on GCC,
but introduces a more reliable way to free optparse_t pointers.

This commit also solves the following memory leaks:

2 missing calls to optparse_del() in loki.c (loki_start_cmd())
6 missing calls to optparse_del() in slash_apm.c (apm_load_cmd(), apm_info_cmd())
1 call to optparse_del() after return in prometheus.c (prometheus_start_cmd())

1 missing call to optparse_del() in builtins.c (slash_builtin_watch())
@johandc

johandc commented Apr 10, 2026

Copy link
Copy Markdown
Contributor

I dont favor the gcc extension. A large portion of our code is flight code which would not accept it, so we might as well use similar pattern. The preferred pattern which is MISRA friendly is the goto cleanup; pattern. This is widely used and improves readability, just like your proposal, but without adding a non complliant compiler extension.

@kivkiv12345

Copy link
Copy Markdown
Contributor Author

That is a fair point, although I recall one time I helped debug undefined behavior stemming from goto cleanup; skipping a variable declaration.

Since making this PR, I have learned that Clang supports __attribute__((cleanup())) as well.

I recall Troels strongly prefers we don't include compiler specific extension usage in open-source libraries, a fair point.
So that's 2 arguments against including it in libparam.

Your argument against CSH is to keep it uniform.
I would still prefer a cleaner solution than goto cleanup; but ultimately I don't have a strong opinion between the 2.

I suspect (hope) the memory leaks mentioned in the description have been fixed since making this PR.

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.

2 participants