rpmbuild: Fix -ts srpm specfile name
#1397
Conversation
This mostly reverts acf5e00 After that commit, the source spec file name from the tarball is not preserved in the src.rpm, and instead looks like rpm-tmp.57WuUk. This mostly reverts the filename handling to the previous state. The spec is extracted from the tar file to a %{_tmppath}/$TMPNAME. This is then renamed to %{_tmppath}/$ORIGINAL_BASENAME. The stated aim of the original commit was to remove creation of _sourcedir and _specdir, which is unchanged. After this revert, _tmppath is used for spec storing where previously before the offending commit _specdir was used. But when I revived that piece the test suite had some failures. Maybe this requires a bigger change, or a full revert of the original patch. Resolves: rpm-software-management#1386 Signed-off-by: Cole Robinson <crobinso@redhat.com>
| FD_t fd = NULL; | ||
| static const char *tryspec[] = { "Specfile", "\\*.spec", NULL }; | ||
|
|
||
| if (!(fd = rpmMkTempFile(NULL, &specFile))) | ||
| specDir = rpmGetPath("%{_tmppath}", NULL); |
There was a problem hiding this comment.
Cosmetic: For clarity, I would move this line to before the specBase = assignment further down, as it's only really used in that block.
| /* mkstemp() can give unnecessarily strict permissions, fixup */ | ||
| mode_t mask; | ||
| umask(mask = umask(0)); | ||
| (void) chmod(specFile, 0666 & ~mask); |
There was a problem hiding this comment.
It seems like this is no longer needed - with a plain mkstemp(3), the file mode of the resulting file is undefined (as per the man page) and so needs to be manually set, however rpmMkTempFile() takes care of that for us.
| FD_t fd = NULL; | ||
| static const char *tryspec[] = { "Specfile", "\\*.spec", NULL }; | ||
|
|
||
| if (!(fd = rpmMkTempFile(NULL, &specFile))) | ||
| specDir = rpmGetPath("%{_tmppath}", NULL); | ||
| if (!(fd = rpmMkTempFile(NULL, &tmpSpecFile))) |
There was a problem hiding this comment.
I'm thinking if it still makes sense to create this as a temp file...
Originally, I suppose, it was done because we didn't want to write the final name into _specdir before being done with it (as to not risk leaving an incomplete file behind in case of a failure), however now that we're keeping the file in _tmppath even after the rename, couldn't we just use the final name from the start?
There was a problem hiding this comment.
My main issue with this patch is that it tries to place a "predictable" filename into a world-writable directory. This is always gets people going about security, but it also can cause unpredictable failures for no good reason (two users trying to do the same thing at the same time, stranger things have happened). That is a no-go.
So we kinda do need a temporary file for this, but we also need a place to put the final spec. The specdir as used by the original code wasn't optimal either as you could end up overwriting your own data with zero warning (has actually happened to me). One "easy" solution would be just using current directory, but with similar risks of overwriting data. So to handle this in a way that cannot randomly fail, or overwrite your data, if you happen to have a file by the same name around, we'd need to create a temporary directory and place the spec (and anything else we might need) there.
There was a problem hiding this comment.
...
The other, simpler option would be going back to creating %_specdir if it doesn't exist. That was not the main target in the 4.16 change anyway, %_sourcedir was.
There was a problem hiding this comment.
Hmm, using %_tmpdir brings on yet another problem: rename() will then fail "randomly" depending on your partitioning scheme and commonly fail because of it, which is also not okay. For such a silly little thing this is annoyingly complicated and subtle...
|
Other than my inline comments, looks good to me! |
|
Submitted an alternative solution as #1453 |
|
Thanks for handling this. We can close this PR. |
|
Thanks @crobinso for the bug report and initial patch, it certainly did help this getting sorted out. |
This mostly reverts acf5e00
After that commit, the source spec file name from the tarball is
not preserved in the src.rpm, and instead looks like rpm-tmp.57WuUk.
This mostly reverts the filename handling to the previous state.
The spec is extracted from the tar file to a %{_tmppath}/$TMPNAME.
This is then renamed to %{_tmppath}/$ORIGINAL_BASENAME.
The stated aim of the original commit was to remove creation of
_sourcedir and _specdir, which is unchanged. After this revert,
_tmppath is used for spec storing where previously before the offending
commit _specdir was used. But when I revived that piece the test
suite had some failures.
Maybe this requires a bigger change, or a full revert of the
original patch.
Resolves: #1386
Signed-off-by: Cole Robinson crobinso@redhat.com