Fix some memory Leak about transactions, loggers, strdups#25
Merged
johannessen merged 4 commits intomajensen:mainfrom Aug 2, 2025
Merged
Conversation
In `new_transaction`, a logger is created by `neo4j_get_logger`. The logger get by this way must be released by `neo4j_logger_release`. Unreleased logger resulted in a memory leak here.
The memory allocated in the transaction mpool will leak without drained.
In `neo4j_config_set_supported_versions` and `new_transaction`, the `strdup`ed vals never free. The especially special one is `tx->mode = ( mode == NULL ? "w" : strdup(mode) )` The ownership of the memory for `tx->mode` is very ambiguous. It's a headache to release it. So I change this to be only static "w" or static "s". If the input `mode` is not equal to "s", then `tx->mode` will be default "w". The `tx->dbname` also has an issue. Though it is defined as `const char *`, which means the tx does not own the string object. However, it is created with `strdup`, which means it owns the string and should free it. So I believe its type is incorrect. An alternative option is to let the dbname just refer the arg `dbname`, which require the caller to ensure the lifetime of the `dbname`. I do not like this. I choose to modify the `tx->mode` to `char *`.
The `neo4j_check_known_hosts` used the `REQUIRE` macro after the `neo4j_get_logger`, resulting in a memory since the `REQUIRE` add a concealed return point without the `neo4j_logger_release`. The `REQUIRE` macro must be used at the beginning of a function. Meanwhile, I find that the `neo4j_std_logger_provider` is not multithread safe. This maintain the linked list and ref count without atomic or lock, which would be very prone to being misused. I think the `neo4j_std_logger_provider` is not intended to be used under a multithread scene, so I add some docs to warn user who may want to share a std logger provider among connections in different thread.
npc1054657282
changed the title
johannessen
approved these changes
Jul 4, 2025
Collaborator
johannessen
left a comment
johannessen
left a comment
There was a problem hiding this comment.
Thanks. Good call on tx->mode and tx->dbname.
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
2 participants
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.
new_transaction, the resources it allocated were all not released inneo4j_free_tx, including the unreleased logger, undrained mpool and unfree strdup.new_transactionis more complicated, because the ownership of the memory fortx->modeis very ambiguous. I try to change this to be only static "w" or static "r". If the inputmodeis not equal to "r", thentx->modewill be default "w".tx->dbnamein transaction is also a problem. If it would free, the dbname must be defined aschar *rather thanconst char *. Considering the transaction owns the dbanme, it should hold the string to be not const since the transaction should take responsibility to free it.neo4j_config_set_supported_versions.neo4j_check_known_hosts, theREQUIREDmacro add a concealed return point without theneo4j_logger_release. The macro must be used at the beginning of a function.