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 race condition in rpmioMkpath #3509

Merged
merged 2 commits into from
Jan 10, 2025
Merged

Conversation

ffesti
Copy link
Contributor

@ffesti ffesti commented Jan 8, 2025

Fix race condition in rpmioMkpath
The previous implementation did stat the directory and created it if not
there. This allowed for a race condition where other rpm instances could
create the directory inbetween. This was observed in the wild for
%{_tmppath}.

Do create the directory unconditionally and if that fails see if the
directory is there already.

Not adding a test case as rpmioMkpath is heavily used all over the the
code base and tests for race conditions don't really fit in the test suite.

Resolves: #3508

@ffesti ffesti requested a review from a team as a code owner January 8, 2025 09:43
@ffesti ffesti requested review from pmatilai and removed request for a team January 8, 2025 09:43
@dmnks dmnks requested review from dmnks and removed request for pmatilai January 8, 2025 09:52
rpmio/rpmio.cc Show resolved Hide resolved
@ffesti
Copy link
Contributor Author

ffesti commented Jan 9, 2025

Yes, this should be ENOENT. Will fix as soon as I have a clean work dir.

See

AT_SETUP([urlhelper missing])
ff

The error: open of https://www.example.com/foo-1.0-1.x86_64.rpm failed: No such file or directory is based on the errno. This looks right.

But errno is only set to ENOENT by accident. The next patch changes that. This patch keeps the same behavior for callers of urlOpen.

@dmnks
Copy link
Contributor

dmnks commented Jan 9, 2025

Oh, right. Re-reading the code with ENOENT in mind (instead of ENOTDIR) suddenly makes it click for me, thanks!

Copy link
Contributor

@dmnks dmnks left a comment

Choose a reason for hiding this comment

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

Otherwise, LGTM. When you fix up the commit message, just hit merge.

Callers use the errno for error reporting. Currently it is set to ENOENT
by accident bubbling up from rpmMkTempFile and rpmioMkpath.

As we need to change rpmioMkpath set errno explicitly to keep the errno
the same for the callers of urlOpen.

Related: rpm-software-management#3508
The previous implementation did stat the directory and created it if not
there. This allowed for a race condition where other rpm instances could
create the directory inbetween. This was observed in the wild for
%{_tmppath}.

Do create the directory unconditionally and if that fails see if the
directory is there already.

Not adding a test case as rpmioMkpath is heavily used all over the the
code base and tests for race conditions don't really fit in the test suite.

Resolves: rpm-software-management#3508
@ffesti ffesti merged commit b3e0c41 into rpm-software-management:master Jan 10, 2025
1 check passed
@ffesti ffesti deleted the 3508 branch January 13, 2025 11:10
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.

Race condition in rpmioMkpath ("Unable to open temp file: File exists")
2 participants