futils_mktmp: don't use umask #6178
Conversation
a4a283c to
2755cd9
Compare
| p_umask(mask = p_umask(0)); | ||
| while (tries--) { | ||
| git_str_sets(path_out, filename); | ||
| git_str_puts(path_out, "_git2_XXXXXX"); |
There was a problem hiding this comment.
We could help mktemp() out a bit by adding a couple of random number so the template string before passing it on.
- git_str_sets(path_out, filename);
- git_str_puts(path_out, "_git2_XXXXXX");
+ git_str_clear(path_out);
+ git_str_printf(path_out, "%s_git2_%04XXXXXXX", filename, rand());It could be useful for systems that base the mktemp string on the PID. (I think Windows does this.) On Linux mktemp() seems to randomize the entire field to create a unique filename.
As for possible DoS attacks I think that a malicious person with write access to the repository can do a bit more damage than just that.
There was a problem hiding this comment.
I wouldn't want to use rand - we can't be guaranteed that the caller ever invoked srand in a meaningful way and we wouldn't want to do it ourselves because they may be relying on an srand seed for some sort of reproducibility.
I appreciate your reminder that this is limited to the repository:
a malicious person with write access to the repository can do a bit more damage than just that.
I'd been thinking about this function as a generic temporary file creation utility; since we don't use this to write into /tmp, I don't think that we need to be too careful about it. I added a note on the header so that we remember its usefulness and merged it manually.
Thanks again for this!
|
|
||
| #define p_close(fd) close(fd) | ||
| #define p_umask(m) umask(m) | ||
| #define p_mktemp(p) mktemp(p) |
There was a problem hiding this comment.
Windows have different implementations for p_mkstemp depending on _MSC_VER, is that needed for p_mktemp too?
Previously, we were using `umask(mask = umask(0))` to fetch the current umask in order to apply it to the desired mode, but this is broken in the presence of threads. There is no portable way to directly fetch umask without mutating it. Instead, add a reimplementation of mkstemp that uses a passed-in mode, instead of explicitly chmodding to 0600 like POSIX requires of mkstemp. Fixes: jmgao/pore#46
Modification of #5350 with
tempnam()mktemp() since tempnam() isn't supported on the macos and windows builds.For some reason the test doesn't fail when used on its own. Maybe something have changed since then?
The Open Group Base Specifications Issue 7 marks tempnam() as obsolescent and states that "The tempnam() function may be removed in a future version."It also limits the prefix to 5 bytes so appending "_git2_" to the prefix is a bit pointless.