[m-rev.] for review: Implement secure temporary file creation for .NET.

[Sebastian Godelet]

Hi Paul,

Thanks for looking into this.
I've actually identified a small issue in the patch I send to the mailing list,
So I've updated my diff, please see https://github.com/Mercury-Language/mercury/pull/40 (with optional .diff|.patch suffix).
Issue was that the buffer size has to be positive on Windows: File.Create(FileName, 0 -> 512,
I haven't changed anything else though.

> -----Original Message-----
> From: Paul Bone [mailto:paul at bone.id.au]
> Sent: Monday, May 09, 2016 08:51
> To: Sebastian Godelet <sebastian.godelet at outlook.com>
> Cc: 'Mercury Reviews' <reviews at lists.mercurylang.org>
> Subject: Re: [m-rev.] for review: Implement secure temporary file creation
> for .NET.
> 
> On Fri, May 06, 2016 at 05:29:27PM +0800, Sebastian Godelet wrote:
> > Hi,
> >
> > I've applied my previous changes to the do_make_temp predicate for C#.
> > While going through the C implementation I noticed that the call to
> > mkstemp is violating the functional contract, namely that XXXXXX needs to
> be at the end of the string. This seems to work since Suffix is "" in the usual
> case.
> > IMHO Suffix just should be removed (as using Suffix != "" will not work at
> the moment anyway).
> 
> Someone else suggested this, however once fixed for C (see my comment
> about mkstemps below) the only unsupported backend is Erlang.  Which can
> probably be fixed easily also.

Well when I went through Mono source code I noticed that they only seem to support the mkstemp function for all platforms; assuming that I actually understand the Mono library resolving mechanism correctly (they certainly have an implementation if the standard C library doesn't support this). Note that for our purposes, we could still try to use mkstemps, since on Windows that code is not executed anyway (even on Mono). So at the moment I also have left out the use of Suffix for the .NET/Windows implementation (but it would of course be easy to add), alternatively we could try to call mkstemps inside a try catch block and revert to the use of mkstemp in case it doesn't exist. I can test this easily.

> 
> > ---
> > Implement secure temporary file creation for .NET.
> >
> > library/io.m:
> >         Re-write do_make_temp/9 for C#, using the same mechanism as
> >         do_make_temp_directory.
> >         Implement Prefix for do_make_temp_directory (C#).
> >         Add a comment on why using Suffix for the C code will cause
> >         problems.
> >
> > diff --git a/library/io.m b/library/io.m index 3b96cef..e0e5e9e 100644
> > --- a/library/io.m
> > +++ b/library/io.m
> > @@ -10560,7 +10570,7 @@ import java.util.Random;
> >      MR_Word filename_word;
> >      int     flags;
> >
> > -    len = strlen(Dir) + 1 + 5 + 6 + strlen(Suffix) + 1;
> > +    len = strlen(Dir) + strlen(Sep) + 5 + 6 + strlen(Suffix) + 1;
> 
> Good catch!
> 
> > @@ -10744,6 +10843,9 @@ import java.util.Random;  #ifdef
> > MR_HAVE_MKDTEMP
> >      int err;
> >
> > +    /* XXX mkdtemp assumes that XXXXXX is at the end of the template,
> > +     * and mkstemps is used for this purpose instead.
> > +     */
> 
> Although mkstemps is non-standard, it's still a realistic alternative since we
> already support systems without mkstemp.  So we'd continue to support two
> implementations: generate names ourselves and use open / mkstemps. - I'll
> make this a separate proposal.

For the make_temp_file, should there be always two versions? : One that creates a temporary file, and one that creates and opens a tempory file? The implementations of those two could and should be different, currently we create a file and then close the stream, but for example on Windows it is much more efficient to open a stream temporary and let the system delete it when the file handle is closed. Like the O_TMPFILE - Linux only? - flag I mentioned earlier or the FILE_ATTRIBUTE_TEMPORARY the FILE_FLAG_DELETE_ON_CLOSE Windows equivalent (although we use _wopen at the moment instead of CreateFile).

I would also volunteer to implement the mkdtemp(s) implementation for Windows if you don't mind. It would be very similar to the .NET/Windows make_temp_directory implementation (i.e. use the CreateDirectoryW Win32 call, etc.). After all that is what the .NET API is wrapping in the first place.

> 
> Your changes look good.  I'll give them a quick test and commit them.

Thanks!

Sebastian.

> 
> Cheers.
> 
> 
> --
> Paul Bone