lib/commit: Unref repo on success

[GeorgesStavracas]

Commit 540e60c3 introduced _ostree_repo_auto_transaction_new(), a private constructor to OstreeRepoAutoTransaction, by factoring out some code from _ostree_repo_auto_transaction_start(). This factored code increased the refcount of the 'repo' variable.

Subsequent commit 71304e854c made ostree_repo_prepare_transaction() use ths newly introduced constructor. However, in this function, the happy path assumed no ref was taken, and therefore did not unref it. Commit 71304e854c didn't add the corresponding unref either.

This leaks a reference to OstreeRepo when calling ostree_repo_prepare_transaction().

Plug this leak by using g_clear_object() to clear the repo field of OstreeRepoAutoTransaction, instead of simply setting it to NULL.

Closes https://github.com/flatpak/flatpak/issues/4928

[openshift-ci[bot]]

Hi @GeorgesStavracas. Thanks for your PR.

I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[GeorgesStavracas]

I do not understand what in this commit could cause the test failures in Debian Testing specifically, and not others. However, it seems like Debian Testing is failing on the main branch as well.

[GeorgesStavracas]

@smcv because I nominally mentioned 2 commits of yours, I would like to know if you agree with this change, or if I missed something

[GeorgesStavracas]

One way you can verify the correctness of this change is through the original reproducer in Flatpak, something like this:

• Open GNOME Software
• Open a terminal and run $ watch -n 1 "lsof -p $(pidof gnome-software) | grep repo | wc -l"
• In GNOME Software, go to the Updates tab and click the refresh button
• Refresh a few more times

Without this branch, the terminal window will show an increasing number of open file descriptions. With this branch, the number will rise while refreshing, and reduce again after refreshing is finished.

[dbnicholson]

/ok-to-test

[smcv]

The issue I was fixing in the commits you mentioned is that the other field in the txn, the refcount, was uninitialized (which would usually mean the intended cleanup wasn't done, unless it was accidentally 1, in which case the right thing happened, or it was accidentally 0, in which case there would probably be assertion failures).

I believe 540e60c3 was correct: it's purely refactoring.

In 71304e85, yes, you're right that there was an unintended semantic change from borrowing a ref (before 71304e85) to incrementing the refcount (after 71304e85). This was wrong before 71304e85, because the txn thought it owned the reference, and so did ostree_repo_prepare_transaction(): only one of them can be right. However, there was a compensating error: because the refcount was uninitialized, this usually didn't matter, because the refcount would usually never reach zero and so the txn would never be properly freed.

After 71304e85, yes, this was leaking one reference to the repo in the happy path, and your change looks like the correct fix for this.