[m-rev.] for review: Improve Mercury's temporary files and directories predicates.

[Julien Fischer]

Hi Paul,

On Wed, 13 Apr 2016, Paul Bone wrote:

> For review by anyone.
> 
> I don't really know C# and have even less knowledge of Erlang.  I've not
> implemented these for the Erlang backend and a reviewer may wish to
> scrutinze the C# code in particular.
> 
> Thanks.
> 
> ---
> 
> Improve Mercury's temporary files and directories predicates.
>
>  + Add support for retriving the system/user's temporary directory.

s/retriving/retrieving/

>  + Add support for creating temporary directories.
> 
> library/io.m:
>     Add temp_directory/3
>
>     Add make_temp_directory/3 and make_temp_directory/5.
>
>     make_temp_file/3 and make_temp_file/5 now return the full pathname on
>     the Java backends.
>
>     On the Java backend make_temp_file/3 and make_temp_file/5 now create
>     files chmod 600, which is necessary when the temporary files are


Add "on Unix-like systems" after "chmod 600" there.

>     security sensative.

s/sensative/sensitive/

>     Document the limitations of these calls for the C# and Erlang backends.
> 
> configure.ac:
> runtime/mercury_conf.h.in:
>     Check for mkdtemp(3).
> 
> NEWS:
>     Announce the new predicates.

> diff --git a/configure.ac b/configure.ac
> index d282008..037a6bd 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1316,8 +1316,8 @@ mercury_check_for_functions \
>          grantpt unlockpt ptsname tcgetattr tcsetattr ioctl \
>          access sleep opendir readdir closedir mkdir symlink readlink \
>          gettimeofday setenv putenv _putenv posix_spawn sched_setaffinity \
> -        sched_getaffinity sched_getcpu sched_yield mkstemp setrlimit \
> -        fma
> +        sched_getaffinity sched_getcpu sched_yield mkstemp mkdtemp
> +        setrlimit fma
>
>  mercury_check_for_stdio_functions \
>          snprintf _snprintf vsnprintf _vsnprintf
> diff --git a/library/io.m b/library/io.m
> index b685315..3195532 100644
> --- a/library/io.m
> +++ b/library/io.m
> @@ -1312,8 +1312,65 @@
>      % It is the responsibility of the program to delete the file when it is
>      % no longer needed.
>      %
> -    % The file will reside in an implementation-dependent directory.
> -    % For current Mercury implementations, it is determined as follows:
> +    % The file is placed in the directory returned by temp_directory/3.
> +    %
> +    % This is insecure on the Erlang backend, file permissions are not set
> +    % properly.

     ... as it does not set file permissions properly.

> +    %
> +:- pred make_temp(string::out, io::di, io::uo) is det.
> +
> +    % make_temp(Dir, Prefix, Name, !IO) creates an empty file whose
> +    % name is different to the name of any existing file. The file will reside
> +    % in the directory specified by `Dir' and will have a prefix using up to
> +    % the first 5 characters of `Prefix'. Name is bound to the name of the

You've quoted `Dir' and `Prefix', but not Name.

> +    % file. It is the responsibility of the program to delete the file
> +    % when it is no longer needed.

Why do we phase that way?  This might be better:

     It is the responsibility of the caller to delete the file when it
     is no longer required.

> +    %
> +    % The C# backend has the following limitations:
> +    %   + Dir is ignored.
> +    %   + Prefix is ignored.
> +    %
> +    % This is insecure on the Erlang backend, file permissions are not set
> +    % properly.

Ditto.

...

> @@ -10310,25 +10361,12 @@ io.setenv(Var, Value) :-
>
>  %---------------------------------------------------------------------------%
> 
> -    % We need to do an explicit check of TMPDIR because not all
> -    % systems check TMPDIR for us (eg Linux #$%*@&).
> -io.make_temp(Name, !IO) :-
> -    Var = ( if dir.use_windows_paths then "TMP" else "TMPDIR" ),
> -    io.get_environment_var(Var, Result, !IO),
> -    (
> -        Result = yes(Dir)
> -    ;
> -        Result = no,
> -        ( if dir.use_windows_paths then
> -            Dir = dir.this_directory
> -        else
> -            Dir = "/tmp"
> -        )
> -    ),
> -    io.make_temp(Dir, "mtmp", Name, !IO).
> +make_temp(Name, !IO) :-
> +    temp_directory(Dir, !IO),
> +    make_temp(Dir, "mtmp", Name, !IO).
> 
> -io.make_temp(Dir, Prefix, Name, !IO) :-
> -    io.do_make_temp(Dir, Prefix, char_to_string(dir.directory_separator),
> +make_temp(Dir, Prefix, Name, !IO) :-
> +    do_make_temp(Dir, Prefix, char_to_string(dir.directory_separator),
>          Name, Err, Message, !IO),
>      ( if Err = 0 then
>          true

...

> @@ -10591,6 +10629,151 @@ io.make_temp(Dir, Prefix, Name, !IO) :-
>          end.
>  ").
> 
> +%-----------------------------------------------------------------------%
> +
> +:- pred io.do_make_temp_directory(string::in, string::in, string::in,
> +    string::out, int::out, string::out, io::di, io::uo) is det.
> +
> +:- pragma foreign_proc("C",
> +    io.do_make_temp_directory(Dir::in, Prefix::in, Sep::in, DirName::out,
> +        Error::out, ErrorMessage::out, _IO0::di, _IO::uo),
> +    [will_not_call_mercury, promise_pure, tabled_for_io,
> +        does_not_affect_liveness],
> +"
> +#ifdef MR_HAVE_MKDTEMP
> +    int err;
> +
> +    DirName = MR_make_string(MR_ALLOC_ID, ""%s%s%.5sXXXXXX"",
> +        Dir, Sep, Prefix);
> +    DirName = mkdtemp(DirName);
> +    if (DirName == NULL) {
> +        ML_maybe_make_err_msg(MR_TRUE, errno,
> +            ""error creating temporary directory: "", MR_ALLOC_ID,
> +            ErrorMessage);
> +        Error = -1;
> +    } else {
> +        ErrorMessage = MR_make_string_const("""");
> +        Error = 0;
> +    }
> +#else
> +    #warning ""Your system does not have mkdtemp""
> +    Error = -1;
> +    ErrorMessage =
> +        MR_make_string_const(""Your system does not have mkdtemp"");
> +    DirName = MR_make_string_const("""");
> +#endif /* HAVE_MKDTEMP */
> +").

Since it isn't going to work on some platforms (e.g. native Windows), please
add a predicate have_make_temp_directory/0 that allows the programer to test
whether a working make_temp_directory is available.  (See the handling of
float.fma/3, for example.)

(W.r.t Windows, I'm fairly certain it should be possible to implement
equivalent functionality using the Win32 API, but I can't tell you how to do it
off the top of my head.)

> +:- pragma foreign_proc("C#",
> +    io.do_make_temp_directory(Dir::in, _Prefix::in, _Sep::in, DirName::out,
> +        Error::out, ErrorMessage::out, _IO0::di, _IO::uo),
> +    [will_not_call_mercury, promise_pure, tabled_for_io, thread_safe],
> +"{
> +    try {
> +        DirName = Path.Combine(Dir, Path.GetRandomFileName());
> +        /*
> +         * This is not secure:
> +         *   1. We cannot set permissions
> +         *   2. We cannot atomically test for and create a directory
> +         */
> +        Directory.CreateDirectory(DirName);
> +        Error = 0;
> +        ErrorMessage = """";
> +    }
> +    catch (System.Exception e)
> +    {
> +        DirName = """";
> +        Error = -1;
> +        ErrorMessage = e.Message;
> +    }
> +}").

Return a bool flag to indicate an error here -- see my comments on
system_temp_directory below.

> +:- pragma foreign_proc("Java",
> +    io.do_make_temp_directory(Dir::in, Prefix::in, _Sep::in, DirName::out,
> +        Error::out, ErrorMessage::out, _IO0::di, _IO::uo),
> +    [will_not_call_mercury, promise_pure, tabled_for_io, thread_safe,
> +        may_not_duplicate],
> +"
> +    try {
> +        Path dir_path, new_file;
> +
> +        if (Prefix.length() > 5) {
> +            // The documentation for io.make_temp says that we should only use
> +            // the first five characters of Prefix.
> +            Prefix = Prefix.substring(0, 5);
> +        }
> +        dir_path = Paths.get(Dir);
> +        new_file = Files.createTempDirectory(dir_path, Prefix);
> +        DirName = new_file.toAbsolutePath().toString();
> +        Error = 0;
> +        ErrorMessage = """";
> +    } catch (java.lang.Exception e) {
> +        DirName = """";
> +        Error = -1;
> +        ErrorMessage = e.toString();
> +    }
> +").
> +
> +%---------------------------------------------------------------------------%
> +
> +temp_directory(Dir, !IO) :-
> +    % If using a Java or C# backend then use their API to get the location of
> +    % temporary files.

    *the* Java or C# backends

> +    system_temp_dir(Dir0, OK, !IO),
> +    ( if OK = 1 then
> +        Dir = Dir0
> +    else
> +        % Either this is not a Java or C# grade or the Java or C# backend
> +        % couldn't determine the temporary directory.
> +        %
> +        % We need to do an explicit check of TMPDIR because not all
> +        % systems check TMPDIR for us (eg Linux #$%*@&).
> +        Var = ( if dir.use_windows_paths then "TMP" else "TMPDIR" ),
> +        get_environment_var(Var, Result, !IO),
> +        (
> +            Result = yes(Dir)
> +        ;
> +            Result = no,
> +            ( if dir.use_windows_paths then
> +                Dir = dir.this_directory
> +            else
> +                Dir = "/tmp"
> +            )
> +        )
> +    ).
> +
> +:- pred system_temp_dir(string::out, int::out, io::di, io::uo) is det.

Use a bool rather than an int for the second argument.
(bool.YES and bool.NO in Java; mr_bool.YES and mr_bool.NO in C#)

Julien.