koji

#4012 rmtree: use fork
Merged by tkopecek. Opened by mikem.
mikem/koji rmtree-bug-v4b  into  master

Download 4012.patch

These changes work around a thread safety issue in our rmtree
implementation, which uses chdir to traverse the directory tree.
Using chdir resolves issues deleting paths longer than PATH_MAX, but
makes the code inherently unsafe in a threaded environment.

Now, the main rmtree function uses fork to perform the actions in a
dedicated process.
To avoid possible locking issues with the logging module, we introduce a
simple proxy logger for the subprocess.

Fixes: https://pagure.io/koji/issue/3755
Fixes: https://pagure.io/koji/issue/2481

For historical context see:
https://pagure.io/koji/issue/201
https://pagure.io/koji/issue/2481
https://pagure.io/koji/issue/2714

This could probably stand some additional unit test coverage, and I may want to tweak how kojira uses the function to avoid a double fork, but worth getting some review.

@tkopecek this is not the version I was telling you about on Friday. That one is much more change. For an initial fix, this seems safer.

2 new commits added

  • cleanup
  • more testcases

typo: not shouldn't be here

Otherwise :thumbsup:

2 new commits added

  • Align our cwd tracking with os
  • avoid double fork in kojira rmtree

I adjusted how kojira calls this to avoid the double fork. Testing that locally exposed an issue with the cwd check that is now fixed.

typo: not shouldn't be here

I didn't see that before I updated, and now I'm having a hard time telling which line this is about. Pagure says "517 of tests/test_lib/test_utils.py", but even at the previous ref this is not any line that I changed.

Perhaps this bit in test_rmtree_relative?

if not os.path.exists(dirname):
    raise Exception('test directory not removed')

1 new commit added

  • fix samefile check and relative path test

Regardless, that particular not was masking an actual issue. Thanks!

At this point, I've addressed the two things I mentioned above, and local kojira testing looks good.

:thumbsup:

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

1 new commit added

  • correct a few comment lines

Metadata Update from @mfilip:
- Pull-request tagged with: testing-done

Commit d3316540 fixes this pull-request

Pull-Request has been merged by tkopecek
Metadata
None

testing-done testing-ready

Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.