Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[WIP] Create temporary files w/o race conditions whenever possible (fixes #1145) #1177

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[WIP] Create temporary files w/o race conditions whenever possible (fixes #1145) #1177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4763104
make_temp_file__: New function.
Jun 24, 2022
be364a7
make_temp_dir__: New function.
Jun 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions inst/@sym/function_handle.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
%% Copyright (C) 2014-2019 Colin B. Macdonald
%% Copyright (C) 2022 Alex Vong
%%
%% This file is part of OctSymPy.
%%
Expand Down Expand Up @@ -255,13 +256,13 @@
%!test
%! % output to disk
%! fprintf('\n')
%! temp_path = make_temp_dir__ (tempdir (), 'octsympy-temp-dir-');
%! if (exist ('OCTAVE_VERSION', 'builtin'))
%! temp_file = tempname('', 'oct_');
%! temp_file = [temp_path '/oct_temp_file'];
%! else
%! temp_file = tempname();
%! temp_file = [temp_path '/temp_file'];
%! end
%! % allow loading function from temp_file
%! [temp_path, ans, ans] = fileparts(temp_file);
%! addpath(temp_path);
%! f = function_handle(2*x*y, 2^x, 'vars', {x y z}, 'file', temp_file);
%! assert( isa(f, 'function_handle'))
Expand All @@ -280,13 +281,13 @@

%!test
%! % output to disk: also works with .m specified
%! temp_path = make_temp_dir__ (tempdir (), 'octsympy-temp-dir-');
%! if (exist ('OCTAVE_VERSION', 'builtin'))
%! temp_file = [tempname('', 'oct_') '.m'];
%! temp_file = [temp_path '/oct_temp_file.m'];
%! else
%! temp_file = [tempname() '.m'];
%! temp_file = [temp_path '/temp_file.m'];
%! end
%! % allow loading function from temp_file
%! [temp_path, ans, ans] = fileparts(temp_file);
%! addpath(temp_path);
%! f = function_handle(2*x*y, 2^x, 'vars', {x y z}, 'file', temp_file);
%! assert( isa(f, 'function_handle'))
Expand Down Expand Up @@ -319,13 +320,13 @@
%! H = [x y z];
%! M = [x y; z 16];
%! V = [x;y;z];
%! temp_path = make_temp_dir__ (tempdir (), 'octsympy-temp-dir-');
%! if (exist ('OCTAVE_VERSION', 'builtin'))
%! temp_file = tempname('', 'oct_');
%! temp_file = [temp_path '/oct_temp_file'];
%! else
%! temp_file = tempname();
%! temp_file = [temp_path '/temp_file'];
%! end
%! % allow loading function from temp_file
%! [temp_path, ans, ans] = fileparts(temp_file);
%! addpath(temp_path);
%! h = function_handle(H, M, V, 'vars', {x y z}, 'file', temp_file);
%! assert( isa(h, 'function_handle'))
Expand Down
3 changes: 2 additions & 1 deletion inst/@sym/sym.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
%% Copyright (C) 2014-2019, 2022 Colin B. Macdonald
%% Copyright (C) 2016 Lagu
%% Copyright (C) 2022 Alex Vong
%%
%% This file is part of OctSymPy.
%%
Expand Down Expand Up @@ -953,7 +954,7 @@
%! syms x
%! y = 2*x;
%! a = 42;
%! myfile = tempname ();
%! [fd, myfile] = make_temp_file__ (tempdir (), 'octsympy-myfile-');
%! save (myfile, 'x', 'y', 'a')
%! clear x y a
%! load (myfile)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks scary: fd is an open file id...? not closed? does save open another fd on the same file...?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, should have closed the unused fd here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but then this is no better than any other racy way... i.e., does this offer any benefit over tempname?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
50 changes: 50 additions & 0 deletions inst/make_temp_dir__.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
%% Copyright (C) 2022 Alex Vong
%%
%% This file is part of OctSymPy.
%%
%% OctSymPy is free software; you can redistribute it and/or modify
%% it under the terms of the GNU General Public License as published
%% by the Free Software Foundation; either version 3 of the License,
%% or (at your option) any later version.
%%
%% This software is distributed in the hope that it will be useful,
%% but WITHOUT ANY WARRANTY; without even the implied warranty
%% of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
%% the GNU General Public License for more details.
%%
%% You should have received a copy of the GNU General Public
%% License along with this software; see the file COPYING.
%% If not, see <https://www.gnu.org/licenses/>.

%% -*- texinfo -*-
%% @documentencoding UTF-8
%% @deftypefun {@var{path} =} make_temp_dir__ (@var{tmpdir}, @var{prefix})
%% Try to create temporary directory in temporary directory @var{tmpdir} with
%% prefix @var{prefix} in the most secure and portable way possible.
%%
%% This function is not really intended for end users.
%%
%% @end deftypefun


function path = make_temp_dir__ (tmpdir, prefix)
cmd = {'import tempfile'
'(tmpdir, prefix) = _ins'
'return tempfile.mkdtemp(dir=tmpdir, prefix=prefix)'};
path = pycall_sympy__ (cmd, tmpdir, prefix);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a huge fan of having temp dir creation depend on Python...!

Is this really not do-able on Octave?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Since cygwin / msys2 is part of octave in windows (as indicated in #1182), I think we can use the mktemp program.

end


%!test
%! % general test
%! path = make_temp_dir__ (tempdir (), 'octsympy-');
%! assert (~isempty (strfind (path, tempdir ())));
%! assert (~isempty (strfind (path, 'octsympy-')));
%! assert (isfolder (path));
%! assert (isequal (ls (path), ''));
%! assert (rmdir (path));

%!error <Python exception: FileNotFoundError>
%! if (exist ('OCTAVE_VERSION', 'builtin'))
%! make_temp_dir__ ('/nonexistent', '');
%! end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshedding here but seems to me this should verify that /nonexistent is in fact non-existent after line 47.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Debian-based distros use /nonexistent as a convention, but we should make sure it's indeed non-existent

78 changes: 78 additions & 0 deletions inst/make_temp_file__.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
%% Copyright (C) 2022 Alex Vong
%%
%% This file is part of OctSymPy.
%%
%% OctSymPy is free software; you can redistribute it and/or modify
%% it under the terms of the GNU General Public License as published
%% by the Free Software Foundation; either version 3 of the License,
%% or (at your option) any later version.
%%
%% This software is distributed in the hope that it will be useful,
%% but WITHOUT ANY WARRANTY; without even the implied warranty
%% of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
%% the GNU General Public License for more details.
%%
%% You should have received a copy of the GNU General Public
%% License along with this software; see the file COPYING.
%% If not, see <https://www.gnu.org/licenses/>.

%% -*- texinfo -*-
%% @documentencoding UTF-8
%% @deftypefun {[@var{fd}, @var{filename}] =} make_temp_file__ (@var{tmpdir}, @var{prefix})
%% Try to create temporary file in temporary directory @var{tmpdir} with prefix
%% @var{prefix} in the most secure and portable way possible.
%%
%% This function is not really intended for end users.
%%
%% @end deftypefun


%% This function is used in private/python_ipc_*.m
%% Assume Python IPC is not initialized yet

function [fd, filename] = make_temp_file__ (tmpdir, prefix)
if (exist ('OCTAVE_VERSION', 'builtin'))
template = [tmpdir '/' prefix 'XXXXXX'];
[fd, filename, msg] = mkstemp (template);
if (fd == -1 || isequal (filename, ''))
error ('make_temp_file__: cannot create temp file: %s', msg);
end

elseif (usejava ('jvm')) % java is required when not running octave
attrs = javaArray ('java.nio.file.attribute.FileAttribute', 0);
path = javaMethod ('createTempFile', 'java.nio.file.Files',
prefix, '',
attrs);
filename = javaMethod ('toString', path);
fd = fopen (filename, 'r+');

else
error ('make_temp_file__: cannot create temp file: please enable java');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is "please enable java or use GNU Octave"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we should recommend Octave as well

end
end


%!test
%! % general test
%! [fd, filename] = make_temp_file__ (tempdir (), 'octsympy-');
%! assert (~isempty (strfind (filename, tempdir ())));
%! assert (~isempty (strfind (filename, 'octsympy-')));
%! assert (mod (fd, 1) == 0 && fd > 2);
%! s = 'hello, world';
%! fprintf (fd, s);
%! assert (fclose (fd) == 0);
%! fd2 = fopen (filename);
%! s2 = fgets (fd2);
%! assert (isequal (s, s2));
%! assert (fgets (fd2) == -1);
%! assert (fclose (fd2) == 0);
%! if (exist ('OCTAVE_VERSION', 'builtin'))
%! assert (unlink (filename) == 0);
%! else
%! delete (filename);
%! end

%!error <cannot create temp file>
%! if (exist ('OCTAVE_VERSION', 'builtin'))
%! make_temp_file__ ('/nonexistent', '');
%! end
6 changes: 4 additions & 2 deletions inst/private/python_ipc_system.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
%% Copyright (C) 2014-2016, 2022 Colin B. Macdonald
%% Copyright (C) 2022 Alex Vong
%%
%% This file is part of OctSymPy.
%%
Expand Down Expand Up @@ -108,12 +109,13 @@
%% Generate a temp .py file then execute it with system()
% can be useful for debugging, or if "python -c" fails for you
if (isempty (tmpfilename))
tmpfilename = [tempname() '_octsympytmp.py'];
[fd, tmpfilename] = make_temp_file__ (tempdir (), 'octsympy-tmpfile-');
if (verbose)
disp (['This session will use the temp file "' tmpfilename '"'])
end
else
fd = fopen (tmpfilename, 'w');
end
fd = fopen (tmpfilename, 'w');
fprintf(fd, '# temporary autogenerated code\n\n');
% we just added two more lines at the top
info.prelines = info.prelines + 2;
Expand Down