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

Race condition in rpmioMkpath ("Unable to open temp file: File exists") #3508

Closed
champtar opened this issue Jan 7, 2025 · 0 comments · Fixed by #3509
Closed

Race condition in rpmioMkpath ("Unable to open temp file: File exists") #3508

champtar opened this issue Jan 7, 2025 · 0 comments · Fixed by #3509
Assignees
Labels

Comments

@champtar
Copy link

champtar commented Jan 7, 2025

Describe the bug
When running multiple rpmbuild in parallel with a non existing %{_tmppath},
if you are lucky enough you will get an error Unable to open temp file: File exists

To Reproduce
Race condition are always a pain to reproduce
-> Code review

Expected behavior
multiple rpmioMkpath in // do not fail

Output

Unable to open temp file: File exists

Environment

  • OS / Distribution: EL9.5
  • Version: rpm-4.16.1.3-34.el9.x86_64

Additional context

rpm/build/build.cc

Lines 200 to 202 in cae8ef5

fd = rpmMkTempFile(spec->rootDir, &scriptName);
if (Ferror(fd)) {
rpmlog(RPMLOG_ERR, _("Unable to open temp file: %s\n"), Fstrerror(fd));

rpm/rpmio/rpmfileutil.cc

Lines 75 to 108 in cae8ef5

FD_t rpmMkTempFile(const char * prefix, char **fn)
{
const char *tpmacro = "%{_tmppath}"; /* always set from rpmrc */
char *tempfn;
static int _initialized = 0;
FD_t tfd = NULL;
if (!prefix) prefix = "";
/* Create the temp directory if it doesn't already exist. */
if (!_initialized) {
_initialized = 1;
tempfn = rpmGenPath(prefix, tpmacro, NULL);
if (rpmioMkpath(tempfn, 0755, (uid_t) -1, (gid_t) -1))
goto exit;
free(tempfn);
}
tempfn = rpmGetPath(prefix, tpmacro, "/rpm-tmp.XXXXXX", NULL);
tfd = rpmMkTemp(tempfn);
if (tfd == NULL || Ferror(tfd)) {
rpmlog(RPMLOG_ERR, _("error creating temporary file %s: %m\n"), tempfn);
goto exit;
}
exit:
if (tfd != NULL && fn)
*fn = tempfn;
else
free(tempfn);
return tfd;
}

I don't get error creating temporary file error, only Unable to open temp file: File exists error, so the error is from rpmioMkpath when trying to create %{_tmppath}

rpm/rpmio/rpmfileutil.cc

Lines 125 to 131 in cae8ef5

rc = stat(d, &st);
if (rc) {
if (errno != ENOENT)
goto exit;
rc = mkdir(d, mode);
if (rc)
goto exit;

If another process create the folder between stat and mkdir we get the error

@ffesti ffesti self-assigned this Jan 8, 2025
@pmatilai pmatilai added the bug label Jan 8, 2025
@pmatilai pmatilai added this to RPM Jan 8, 2025
@github-project-automation github-project-automation bot moved this to Backlog in RPM Jan 8, 2025
ffesti added a commit to ffesti/rpm that referenced this issue Jan 8, 2025
Callers use the errno for error reporting. Currently it is set to ENOTDIR
by accident bubbling up from rpmMkTempFile and rpmioMkpath.

As we need to change rpmioMkpath set errno explicitly.

Related: rpm-software-management#3508
ffesti added a commit to ffesti/rpm that referenced this issue Jan 8, 2025
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 added a commit to ffesti/rpm that referenced this issue Jan 8, 2025
Callers use the errno for error reporting. Currently it is set to ENOTDIR
by accident bubbling up from rpmMkTempFile and rpmioMkpath.

As we need to change rpmioMkpath set errno explicitly.

Related: rpm-software-management#3508
ffesti added a commit to ffesti/rpm that referenced this issue Jan 8, 2025
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 added a commit to ffesti/rpm that referenced this issue Jan 10, 2025
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
ffesti added a commit to ffesti/rpm that referenced this issue Jan 10, 2025
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 added a commit that referenced this issue Jan 10, 2025
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: #3508
ffesti added a commit that referenced this issue Jan 10, 2025
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
@github-project-automation github-project-automation bot moved this from Backlog to Done in RPM Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants