[vlc-devel] [PATCH] record: use vlc_mkstemp instead of tempnam

[Lyndon Brown]

On Tue, 2020-09-29 at 08:22 +0200, Steve Lhomme wrote:
> On 2020-09-29 6:06, Lyndon Brown wrote:
> > On Mon, 2020-09-28 at 07:48 +0200, Steve Lhomme wrote:
> > > On 2020-09-27 22:30, Lyndon Brown wrote:
> > > > +            psz_file = strdup( DIR_SEP"tmp"DIR_SEP"vlc-
> > > > recording.XXXXXX" );
> > > 
> > > I'm pretty sure /tmp doesn't exist on Windows. And it will
> > > definitely
> > > not be cleaned by the system when the user tells the OS to clean
> > > the
> > > temporary files.
> > 
> > Hi :)
> > 
> > FWIW, just to nitpick, on Windows it wouldn't be /tmp, we'd be
> > talking
> > about "\tmp\foo" due to DIR_SEP being a backslash on Windows, which
> > would be interpreted as being <current-device>\tmp\foo.
> > 
> > Yes indeed there's a problem with respect to Windows; This wasn't a
> > failure to recognise the problem there, I've simply completely
> > forgotten that when I originally made the patch last year that I
> > intended to raise this question of what we should do here wrt.
> > Windows
> > when I eventually submitted it, which is coming back to me now.
> > Apologies for forgetting and thanks for catching it.
> > 
> > The behaviour of the tempnam() function call being replaced, per
> > the
> > man page, is that it follows a set of defined rules to determine
> > where
> > to create the file. The mkstemp() function and friends don't do
> > this I
> > don't think (the man page is not perfectly clear), and certainly
> > the
> > implementation of vlc_mkstemp() in src/text/filesystem.c doesn't do
> > any
> > such thing.
> 
> I think using tmpnam() to generate a temporary name is the way to
> go. 

No, tempnam() is the existing function we're trying to get away from
because the function is to be avoided for security reasons and we get
compiler warnings about our use of it accordingly.

Quoting from the linux man page (which underlines the first few words):

    "Never use this function.  Use mkstemp(3) or tmpfile(3) instead."

    ...

    "Although  tempnam() generates names that are difficult to guess, it is nevertheless possi‐
    ble that between the time that tempnam() returns a pathname, and the time that the program
    opens it, another program might create that pathname using open(2), or create it as a sym‐
    bolic link.  This can lead to security  holes.   To  avoid  such  possibilities,  use  the
    open(2) O_EXCL flag to open the pathname.  Or better yet, use mkstemp(3) or tmpfile(3)."

Similarly in Microsoft documentation, first the tempnam() documentation
([1]) tells us that it's a deprecated redirect to _tempnam(), then the
_tempnam() documentation ([2]) tells us:

    "Generate names you can use to create temporary files. More secure versions of some of these functions are available; see tmpnam_s, _wtmpnam_s."

[1]: 
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/tempnam?view=vs-2019
[2]: 
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/tempnam-wtempnam-tmpnam-wtmpnam?view=vs-2019

> The problem here seems to be where we store temporary files. I think
> the 
> current implementation uses the "current directory". It would be
> better 
> to use the proper system folder to do that. That's the missing part.

You're partly right. The problem is that the tempnam() function
currently in use has the nice feature of being able to automatically
place the file into the system/user temporary directory if you did not
specify a directory, which is not a feature that mkstemp() has. For
mkstemp you need to embed the system/user temporary directory location
yourself into the path/template string you give to it if you want the
file to be created there. We _do_ want to continue placing the temp
file into the system/user temp directory in our switch to mkstemp, and
thus need a solution for defining/fetching that path.

This is something that existing use of mkstemp has failed to address
properly, with test units incorrectly just targetting "/tmp/foo" such
that they will not work properly on Windows, and an es_out_timeshift
function having a weird *nix specific fallback should a directory not
be specified.

So the key problem, affecting both this patch and other existing use of
mkstemp is simply how do we best define or fetch that per-system path.
Once we've solved that, we just need to put it to use.

Of course an alternative to switching to mkstemp(), as mentioned in the
quoted *nix documentation, is tmpfile(). This takes no path/template,
it just simply securely creates and opens a unique temporary file (in
the system/user directory I presume), which will be deleted when closed
or program terminates (according to the man page). But this does not
allow templating the filename (do we care?), and it opens in binary
read/write mode, so is that really a suitable alternative that we could
switch all use of tempnam and mkstemp to to avoid the path issue?

> On Windows there is GetTempPath which gives the proper folder to
> use. 
> It's used in vlc_win32_tmpfile() but it gives a FILE*. IMO we should 
> have a core API that gives either the folder or a file path in the 
> temporary folder.

Interesting, GetTempPath might be exactly what we would want for
windows.

For *nix, it seems we should check the $TMPDIR environment variable,
and fallback to "/tmp" if unavailable. ([3]).

[3]: 
https://unix.stackexchange.com/questions/352107/generic-way-to-get-temp-path

So I think we need to introduce a vlc_get_tempdir() type platform
specific function to retrieve the path, then put that to use with
mkstemp, both in existing use of mkstemp and in replacement of tempnam.

Perhaps we should also evaluate replacing use of the custom vlc_mkstemp
implementation used for Windows (src/text/filesystem.c) with use of
GetTempPath + GetTempFileName? The documentation [4] does suggest that
files created with GetTempFileName are not automatically deleted,
though perhaps that just depends upon whether you've had it created in
the system temp directory or not? Since the function just creates the
file and closes the handle used to do so, I would presume that you need
to open the file in exclusive mode afterwards for security. Using this
would avoid all of our custom logic.

[4]: 
https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettempfilenamea

> > I copied use of `DIR_SEP "tmp" DIR_SEP` from other existing use of
> > the
> > function in the codebase, though it seemed wrong in relation to
> > Windows, hence having intended to ask. We have three unit tests
> > with
> > hard coded "/tmp/foo" strings, and this same `DIR_SEP "tmp"
> > DIR_SEP`
> > thing being done in the GetTmpFile() function of es_out_timeshift.c
> > when it's not given a path. (There are also hard coded "/tmp/foo"
> > paths
> > in vlc_memfd() for posix/linux, but at least that's not so
> > problematic).
> > 
> > So what is up with these existing users of vlc_mkstemp() wrt.
> > Windows?
> > Why are these tests hard coding *nix paths? Why does GetTmpFile()
> > use a
> > *nix path if not given a directory path?
> > 
> > And how do we approach using it correctly as a proper replacement
> > for
> > tempnam() in record and vorepository?
> > 
> > It would appear to me that we need to dynamically define the
> > correct
> > standard system temp path to use, either in a hard coded way or via
> > some header, and then use that for our vlc_mkstemp() calls? Right?
> > 
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> > 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel