Improve Setting Permissions of Created Files#2525
Conversation
felixhandte
force-pushed
the
fix-file-permissions-again
branch
2 times, most recently
from
632dd36 to
3dd0e9d
Compare
felixhandte
changed the title
felixhandte
force-pushed
the
fix-file-permissions-again
branch
from
98468f9 to
b924955
Compare
|
Hello! When compressing from stdin, we were a bit surprised to discover than the behavior of zstd changed on Ubuntu 20.04 as a result of CVE-2021-24031 being fixed in security package update Looks like this PR restores the previous behavior when compressing from stdin, so it would be great to have for us. Our current workaround will be to manually chmod the output file, but we might also build a new release of zstd with this patch, if it gets accepted. |
|
The fuzz sanitizer test failures are unrelated to this PR. But there are still some issues left on Windows : |
felixhandte
force-pushed
the
fix-file-permissions-again
branch
from
b924955 to
08b79af
Compare
| zstd tmp --stdout > tmpCompressed # long command format | ||
| println "test : compress to named file" | ||
| rm tmpCompressed | ||
| rm -f tmpCompressed |
There was a problem hiding this comment.
minor:
a variable $RM would be better to propagate this change throughout the script
`open()`'s mode bits are only applied to files that are created by the call. If the output file already exists, but is not readable, the `fopen()` would fail, preventing us from removing it, which would mean that the file would not end up with the correct permission bits. It's not clear to me why the `fopen()` is there at all. `UTIL_isRegularFile()` should be sufficient, AFAICT.
I think in some unix emulation environments on Windows, (cygwin?) mode bits are somehow respected. So we might as well pass them in. Can't hurt.
felixhandte
force-pushed
the
fix-file-permissions-again
branch
from
14167df to
4f9c6fd
Compare
Cyan4973
left a comment
Cyan4973
left a comment
There was a problem hiding this comment.
My only comment is that
I would have replaced all these rm -f in playTests.sh
by a variable $RM
but that's minor.
4 participants
This PR is a follow-up to issues #1630 and #2491 (and their associated PRs #1644 and #2495).
This stack starts by adding tests to (a) codify the expected behavior (which is at present poorly defined) and (b) enforce that zstd implements that behavior.
I expect, once we've settled on the desired behavior, to refactor how zstd sets permissions on created files. In particular, I expect to switch to the
open()+fdopen()pattern suggested in the bug report and possibly remove the trailingchmod()(if we can get the permissions right from the start, why wait until the end to set them?).