[WIP] Create temporary files w/o race conditions whenever possible (fixes #1145)#1177
[WIP] Create temporary files w/o race conditions whenever possible (fixes #1145)#1177alexvong243f wants to merge 2 commits intognu-octave:mainfrom
Conversation
Try to create temporary file in the most secure and portable way possible. Partially fixes gnu-octave#1145. * inst/make_temp_file__.m: New function. * inst/@sym/sym.m: Use it. * inst/private/python_ipc_system.m: Use it.
Try to create temporary directory in the most secure and portable way possible. Partially fixes gnu-octave#1145. * inst/make_temp_dir__.m: New function. * inst/@sym/function_handle.m: Use it.
| fd = fopen (filename, 'r+'); | ||
|
|
||
| else | ||
| error ('make_temp_file__: cannot create temp file: please enable java'); |
There was a problem hiding this comment.
I guess this is "please enable java or use GNU Octave"?
There was a problem hiding this comment.
Sure, we should recommend Octave as well
| %! [fd, myfile] = make_temp_file__ (tempdir (), 'octsympy-myfile-'); | ||
| %! save (myfile, 'x', 'y', 'a') | ||
| %! clear x y a | ||
| %! load (myfile) |
There was a problem hiding this comment.
this looks scary: fd is an open file id...? not closed? does save open another fd on the same file...?
There was a problem hiding this comment.
Agree, should have closed the unused fd here
There was a problem hiding this comment.
but then this is no better than any other racy way... i.e., does this offer any benefit over tempname?
There was a problem hiding this comment.
This is better than tempname because tempname does not choose the filename and create the file in a single step (aka atomic operation). The fd is opened for convenience but it's not essential. The mktemp shell command similarly returns a filename instead of a fd, but it's still secure.
| cmd = {'import tempfile' | ||
| '(tmpdir, prefix) = _ins' | ||
| 'return tempfile.mkdtemp(dir=tmpdir, prefix=prefix)'}; | ||
| path = pycall_sympy__ (cmd, tmpdir, prefix); |
There was a problem hiding this comment.
not a huge fan of having temp dir creation depend on Python...!
Is this really not do-able on Octave?
There was a problem hiding this comment.
AFAIK octave has no function to create temporary directory.
If java is enabled, we can just use java. For *nix, we can use the shell command mktemp -d... So basically we are left with the case of octave without java running on windows. Do you know if all octave builds on windows include java? If it's the case, problem solved!
There was a problem hiding this comment.
Do you know if all octave builds on windows include java?
Java isn't included in the installer of Octave for Windows. Octave just tries to detect if a compatible JRE "happens" to be installed that it can use.
So, you shouldn't rely on being able to use Java.
There was a problem hiding this comment.
I see. Since cygwin / msys2 is part of octave in windows (as indicated in #1182), I think we can use the mktemp program.
| %!error <Python exception: FileNotFoundError> | ||
| %! if (exist ('OCTAVE_VERSION', 'builtin')) | ||
| %! make_temp_dir__ ('/nonexistent', ''); | ||
| %! end |
There was a problem hiding this comment.
Bikeshedding here but seems to me this should verify that /nonexistent is in fact non-existent after line 47.
There was a problem hiding this comment.
Good idea. Debian-based distros use /nonexistent as a convention, but we should make sure it's indeed non-existent
|
I dunno about this...
This bothers me. I guess I'd rather they were private without |
Is it possible to call a private function in bist? We don't need the |
That's bug #38776. |
alexvong243f
changed the title
alexvong243f
changed the title
3 participants
This PR could be too ambitious but adding 2 new functions is the cleanest way I can think of to fix #1145 once and for all. These functions cannot be private because they are needed by tests so we append
__to their names.