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

Copr plugin: Fix resource leak in load_all_configuration #1988

Merged

Conversation

jrohel
Copy link
Contributor

@jrohel jrohel commented Jan 6, 2025

I discovered a resource leak while working on #1893.
There was a missing globfree function call in the CoprConfig::load_all_configuration() method.

One can say that adding the line globfree(&glob_result); after the for loop is enough. But it's not true. The load_copr_config_file method called in the loop can generate an exception. So I rewrote the code to use std::filesystem::directory_iterator.

The "globfree" function call was missing.

One can say that adding the line `globfree(&glob_result);` after the for
loop is enough. But it's not true. The `load_copr_config_file` method
called in the loop can generate an exception. So I rewrote the code
to use `std::filesystem::directory_iterator`.
@jrohel jrohel force-pushed the fix_copr_plugin_res_leak branch from 3fd9df8 to 2201238 Compare January 6, 2025 12:40
@kontura kontura self-assigned this Jan 7, 2025
std::error_code ec;
for (const auto & dentry : std::filesystem::directory_iterator(drop_in_dir, ec)) {
const auto & path = dentry.path();
if (dentry.is_regular_file() && path.extension() == ".conf") {
Copy link
Contributor

Choose a reason for hiding this comment

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

I worry that is_regular_file() ignores symlinks. That would be a regression comparing to glob().

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe symlinks are not ignored.

The doc says that its effectively std::filesystem::is_regular_file(status()) and status says that symlinks are followed.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right. I got confused that the directory_entry has is_symlink() method. I wrote a simple test program and it indeed derefences symlinks.

I only found one issue: If the symlink creates a loop, std::filesystem::filesystem_error exception is thrown without an identification of the file path:

#include <filesystem>
#include <iostream>
int main(int argc, char **argv) {
    std::filesystem::path file {argv[1]};
    std::cout << "file " << file << "\n";
    std::cout << "is_regular_file() " << std::filesystem::is_regular_file(file) << "\n";
    std::cout << "is_symlink() " << std::filesystem::is_symlink(file) << "\n";
    return 0;
}
test@fedora-42:/tmp $ ./a.out symlink_to_self 
file "symlink_to_self"
terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error'
  what():  filesystem error: status: Too many levels of symbolic links [symlink_to_self]
Aborted (core dumped)

Therefore I recommend catching that error and rethrowing it with the file name (if we do not want to ignore it).

Copy link
Contributor

Choose a reason for hiding this comment

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

I cannot read. It reports the file name in the brackets. So. No problem.

@kontura kontura added this pull request to the merge queue Jan 7, 2025
Merged via the queue into rpm-software-management:main with commit 47070ab Jan 7, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants