-
Notifications
You must be signed in to change notification settings - Fork 246
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
switched to secure mkstemp() #460
Conversation
The tests pass on my testing machine (Debian 9.3):
Most failiing tests are due to |
Codecov Report
@@ Coverage Diff @@
## master #460 +/- ##
==========================================
- Coverage 93.03% 90.12% -2.92%
==========================================
Files 232 144 -88
Lines 58668 27304 -31364
Branches 7649 3925 -3724
==========================================
- Hits 54581 24607 -29974
+ Misses 3122 1957 -1165
+ Partials 965 740 -225 Continue to review full report at Codecov.
|
db9f902
to
c054897
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
src/allmydata/test/test_node.py
Outdated
# * appveyor complains about missing 'resource' module on Windows | ||
if 'Darwin' == system(): | ||
import resource | ||
resource.setrlimit(resource.RLIMIT_NOFILE, (2000, -1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this is probably only necessary because the change to mkstemp()
results in a leaked file descriptor per call. If you add a call to close the file descriptor then they won't leak and the process won't run out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it with no success. Currently I can't solve this and would withdraw this PR if there is no silver lining within the next week.
af1ca2d
to
3572f42
Compare
src/allmydata/node.py
Outdated
@@ -273,12 +312,12 @@ def init_tempdir(self): | |||
# tempfile.TemporaryFile) to put large request bodies in the given | |||
# directory. Without this, the default temp dir is usually /tmp/, | |||
# which is frequently too small. | |||
test_name = tempfile.mktemp() | |||
self.tempfile_fd, test_name = tempfile.mkstemp() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the only change in addition to switching to tempfile.mkstemp()
that is needed to make this work is os.close(tempfile_fd)
. This temporary file doesn't seem to be used anywhere beyond the next line so there's no reason to keep the descriptor open. It seems the intention is only to verify that temporary files get created in the right directory. If you revert the context manager related changes and just os.close(tempfile_fd)
here I would expect things to be in good shape.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a similar thought, but wasn't sure. Thank you for helping and pushing.
everything is green \o/ ... thank you @exarkun |
Cool, thanks. Sorry this took a while to get merged! |
Hi there,
I switched out the deprecated
mktemp()
for the secure (and recommended)mkstemp()
.Cheers
tpltnt