1. fixed file descriptors leak in wxFileName::CreateTempFileName()

2. really made it race-safe (provided we have mkstemp())


git-svn-id: https://svn.wxwidgets.org/svn/wx/wxWidgets/trunk@13076 c3d73ce0-8a6f-49c7-b76d-6d57e0e08775

docs/changes.txt

@@ -66,6 +66,8 @@ Unix ports:
 All:
 
 - fixes to the command line parsing error and usage messages
+- modified wxFileName::CreateTempFileName() to open the file atomically
+  (if possible) and, especially, not to leak the file descriptors under Unix
 
 wxMSW:
 

docs/latex/wx/filename.tex

@@ -226,7 +226,7 @@ Set this file name object to the home directory.
 
 \membersection{wxFileName::AssignTempFileName}\label{wxfilenameassigntempfilename}
 
-\func{void}{AssignTempFileName}{\param{const wxString\& }{prefix}}
+\func{void}{AssignTempFileName}{\param{const wxString\& }{prefix}, \param{wxFile *}{fileTemp = {\tt NULL}}}
 
 The function calls \helpref{CreateTempFileName}{wxfilenamecreatetempfilename} to
 create a temporary file and sets this object to the name of the file. If a
@@ -241,16 +241,28 @@ Reset all components to default, uninitialized state.
 
 \membersection{wxFileName::CreateTempFileName}\label{wxfilenamecreatetempfilename}
 
-\func{static wxString}{CreateTempFileName}{\param{const wxString\& }{prefix}}
+\func{static wxString}{CreateTempFileName}{\param{const wxString\& }{prefix}, \param{wxFile *}{fileTemp = {\tt NULL}}}
 
 Returns a temporary file name starting with the given {\it prefix}. If
 the {\it prefix} is an absolute path, the temporary file is created in this
 directory, otherwise it is created in the default system directory for the
 temporary files or in the current directory.
 
-If the function succeeds, the temporary file is actually created (but not
+If the function succeeds, the temporary file is actually created. If\rtfsp
-opened) as well. Under Unix, it will have read and write permissions for the
+{\it fileTemp} is not {\tt NULL}, this file will be opened using the name of
-owner only.
+the temporary file. When possible, this is done in an atomic way ensuring that
+no race condition occurs between the temporary file name generation and opening
+it which could often lead to security compromise on the multiuser systems.
+If {\it fileTemp} is {\tt NULL}, the file is only created, but not opened.
+
+Under Unix, the temporary file will have read and write permissions for the
+owner only to minimize the security problems.
+
+\wxheading{Parameters}
+
+\docparam{prefix}{Prefix to use for the temporary file name construction}
+
+\docparam{fileTemp}{The file to open or {\tt NULL} to just get the name}
 
 \wxheading{Return value}
 

include/wx/filename.h

@@ -38,6 +38,8 @@
 #include "wx/filefn.h"
 #include "wx/datetime.h"
 
+class WXDLLEXPORT wxFile;
+
 // ----------------------------------------------------------------------------
 // constants
 // ----------------------------------------------------------------------------
@@ -208,9 +210,12 @@ public:
     void AssignHomeDir();
     static wxString GetHomeDir();
 
-        // get a temp file name starting with the specified prefix
+        // get a temp file name starting with the specified prefix and open the
-    void AssignTempFileName(const wxString& prefix);
+        // file passed to us using this name for writing (atomically if
-    static wxString CreateTempFileName(const wxString& prefix);
+        // possible)
+    void AssignTempFileName(const wxString& prefix, wxFile *fileTemp = NULL);
+    static wxString CreateTempFileName(const wxString& prefix,
+                                       wxFile *fileTemp = NULL);
 
     // directory creation and removal.
     // if full is TRUE, will try to make each directory in the path.

src/common/file.cpp

@@ -478,7 +478,7 @@ bool wxTempFile::Open(const wxString& strName)
 {
     m_strName = strName;
 
-    m_strTemp = wxFileName::CreateTempFileName(strName);
+    m_strTemp = wxFileName::CreateTempFileName(strName, &m_file);
 
     if ( m_strTemp.empty() )
     {
@@ -486,13 +486,6 @@ bool wxTempFile::Open(const wxString& strName)
         return FALSE;
     }
 
-    // actually open the file now (it must already exist)
-    if ( !m_file.Open(m_strTemp, wxFile::write) )
-    {
-        // opening existing file failed?
-        return FALSE;
-    }
-
 #ifdef __UNIX__
     // the temp file should have the same permissions as the original one
     mode_t mode;

src/common/filename.cpp

@@ -456,9 +456,9 @@ wxString wxFileName::GetHomeDir()
     return ::wxGetHomeDir();
 }
 
-void wxFileName::AssignTempFileName( const wxString& prefix )
+void wxFileName::AssignTempFileName(const wxString& prefix, wxFile *fileTemp)
 {
-    wxString tempname = CreateTempFileName(prefix);
+    wxString tempname = CreateTempFileName(prefix, fileTemp);
     if ( tempname.empty() )
     {
         // error, failed to get temp file name
@@ -471,7 +471,8 @@ void wxFileName::AssignTempFileName( const wxString& prefix )
 }
 
 /* static */
-wxString wxFileName::CreateTempFileName(const wxString& prefix)
+wxString
+wxFileName::CreateTempFileName(const wxString& prefix, wxFile *fileTemp)
 {
     wxString path, dir, name;
 
@@ -558,13 +559,25 @@ wxString wxFileName::CreateTempFileName(const wxString& prefix)
 
     // can use the cast here because the length doesn't change and the string
     // is not shared
-    if ( mkstemp((char *)path.mb_str()) == -1 )
+    int fdTemp = mkstemp((char *)path.mb_str());
+    if ( fdTemp == -1 )
     {
         // this might be not necessary as mkstemp() on most systems should have
         // already done it but it doesn't hurt neither...
         path.clear();
     }
-    //else: file already created
+    else // mkstemp() succeeded
+    {
+        // avoid leaking the fd
+        if ( fileTemp )
+        {
+            fileTemp->Attach(fdTemp);
+        }
+        else
+        {
+            close(fdTemp);
+        }
+    }
 #else // !HAVE_MKSTEMP
 
 #ifdef HAVE_MKTEMP
@@ -601,10 +614,20 @@ wxString wxFileName::CreateTempFileName(const wxString& prefix)
 
     if ( !path.empty() )
     {
-        // create the file - of course, there is a race condition here, this is
+    }
+#endif // HAVE_MKSTEMP/!HAVE_MKSTEMP
+
+#endif // Windows/!Windows
+
+    if ( path.empty() )
+    {
+        wxLogSysError(_("Failed to create a temporary file name"));
+    }
+    else if ( fileTemp && !fileTemp->IsOpened() )
+    {
+        // open the file - of course, there is a race condition here, this is
         // why we always prefer using mkstemp()...
-        wxFile file;
+        if ( !fileTemp->Open(path, wxFile::write_excl, wxS_IRUSR | wxS_IWUSR) )
-        if ( !file.Open(path, wxFile::write_excl, wxS_IRUSR | wxS_IWUSR) )
         {
             // FIXME: If !ok here should we loop and try again with another
             //        file name?  That is the standard recourse if open(O_EXCL)
@@ -616,14 +639,6 @@ wxString wxFileName::CreateTempFileName(const wxString& prefix)
             path.clear();
         }
     }
-#endif // HAVE_MKSTEMP/!HAVE_MKSTEMP
-
-#endif // Windows/!Windows
-
-    if ( path.empty() )
-    {
-        wxLogSysError(_("Failed to create a temporary file name"));
-    }
 
     return path;
 }