Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory errors in testlibrary and other test code #5691

Merged
merged 10 commits into from Feb 15, 2024

Conversation

smcv
Copy link
Collaborator

@smcv smcv commented Feb 15, 2024

Continuation of #5690, fixing bugs in the test itself. Detected by running tests compiled with AddressSanitizer.

This fixes testlibrary, plus a few issues in other tests that I found before reducing the scope of what I was trying to just testlibrary.

After this has been reviewed I'm intending to continue to clean up AddressSanitizer issues as time permits, but repeatedly running the tests under AddressSanitizer is rather time-consuming, so I'd prefer to land this a piece at a time.


  • tests: Fix a double-free when exercising argument parsing

    g_option_context_add_group() takes ownership of the group that it's
    given, so we can't also free it.

    Fixes: fab0f8e "test-context: Exercise some corner cases for merging filesystems"

  • testlibrary: Don't leak transactions

  • testlibrary: Don't leak several installed references

  • testlibrary: Don't leak FlatpakInstance

  • testlibrary: Don't leak icon data

  • testlibrary: Don't leak an array of related refs

  • testlibrary: Don't leak strings retrieved from remote

    All of these getters are (transfer full) (but note that
    flatpak_remote_get_name() isn't).

  • test_list_remote_related_refs: Don't leak list of subpaths

  • testlibrary: Don't leak list of subpaths

    flatpak_deploy_data_get_subpaths() returns a new array (of unowned
    strings) and flatpak_dir_new_deploy_data() doesn't take ownership.

  • httpcache: Free the GError before exiting

Copy link
Member

@TingPing TingPing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of the pattern:

  g_assert_cmpstr ((s = flatpak_remote_get_default_branch (remote)), ==, "default-branch");
  g_free (s);

being repeated, like maybe that could be pulled out into a function or macro. Not a blocker.

Otherwise looks good, feel free to merge.

@smcv
Copy link
Collaborator Author

smcv commented Feb 15, 2024

maybe that could be pulled out into a function or macro

It should be a macro: if it was a function, then we wouldn't be told the line number of the assertion failure.

Since you say this isn't a blocker, and I could see the design involving an amount of bikeshedding about what is the least ugly way to do it, I'd prefer to do that as a follow-up.

g_option_context_add_group() takes ownership of the group that it's
given, so we can't also free it.

Fixes: fab0f8e "test-context: Exercise some corner cases for merging filesystems"
Signed-off-by: Simon McVittie <smcv@collabora.com>
Signed-off-by: Simon McVittie <smcv@collabora.com>
Signed-off-by: Simon McVittie <smcv@collabora.com>
Signed-off-by: Simon McVittie <smcv@collabora.com>
Signed-off-by: Simon McVittie <smcv@collabora.com>
All of these getters are (transfer full) (but note that
flatpak_remote_get_name() isn't).

Signed-off-by: Simon McVittie <smcv@collabora.com>
Signed-off-by: Simon McVittie <smcv@collabora.com>
flatpak_deploy_data_get_subpaths() returns a new array (of unowned
strings) and flatpak_dir_new_deploy_data() doesn't take ownership.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Signed-off-by: Simon McVittie <smcv@collabora.com>
@smcv smcv merged commit 4b159b1 into flatpak:main Feb 15, 2024
9 checks passed
smcv added a commit to smcv/flatpak that referenced this pull request Feb 15, 2024
Suggested by Patrick during review of flatpak#5691.

Signed-off-by: Simon McVittie <smcv@collabora.com>
smcv added a commit to smcv/flatpak that referenced this pull request Mar 28, 2024
Suggested by Patrick during review of flatpak#5691.

Signed-off-by: Simon McVittie <smcv@collabora.com>
smcv added a commit to smcv/flatpak that referenced this pull request Apr 24, 2024
Suggested by Patrick during review of flatpak#5691.

Signed-off-by: Simon McVittie <smcv@collabora.com>
smcv added a commit that referenced this pull request Apr 25, 2024
Suggested by Patrick during review of #5691.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants