Change how alloc_mmap's TemporaryFile class works. Don't expose

the name at all; instead unlink it at the end of the constructor,
so by the time it is fully constructed it is purely an anonymous
file descriptor.

mkstemp has a weird interface and returns the final name of the
file in its template argument. This prevented us from using a
std::string, since c_str's return is const (and we can't use
&string[0], because that might not be NULL-terminated). This
previously required doing nasty things like explicit new/delete
and using strcpy (the strcpy was what got me started on looking
at this; OpenBSD complains about it, so I was trying to figure
out a good way to remove it).

Instead, use the idea from http://www.gotw.ca/gotw/042.htm, and
use a std::vector to hold the mkstemp argument/result. That works
consistently everywhere, and we don't need to rely on strcpy, and
don't have to worry about memory leaks either. Only minor nit is
having to add an explicit NULL terminator as the std::string
doesn't contain it.

1 files changed, 17 insertions, 10 deletions

diff --git a/src/alloc/alloc_mmap/mmap_mem.cpp b/src/alloc/alloc_mmap/mmap_mem.cpp
index 4a7019ae7..a2059a6ea 100644
--- a/src/alloc/alloc_mmap/mmap_mem.cpp
+++ b/src/alloc/alloc_mmap/mmap_mem.cpp
@@ -6,6 +6,7 @@
 */
 
 #include <botan/internal/mmap_mem.h>
+#include <vector>
 #include <cstring>
 
 #include <sys/types.h>
@@ -44,29 +45,38 @@ void* MemoryMapping_Allocator::alloc_block(u32bit n)
       {
       public:
          int get_fd() const { return fd; }
-         const std::string path() const { return filepath; }
 
          TemporaryFile(const std::string& base)
             {
-            const std::string path = base + "XXXXXX";
+            const std::string mkstemp_template = base + "XXXXXX";
 
-            filepath = new char[path.length() + 1];
-            std::strcpy(filepath, path.c_str());
+            std::vector<char> filepath(mkstemp_template.begin(),
+                                       mkstemp_template.end());
+            filepath.push_back(0); // add terminating NULL
 
             mode_t old_umask = ::umask(077);
-            fd = ::mkstemp(filepath);
+            fd = ::mkstemp(&filepath[0]);
             ::umask(old_umask);
+
+            if(fd == -1)
+               throw MemoryMapping_Failed("Temporary file allocation failed");
+
+            if(::unlink(&filepath[0]) != 0)
+               throw MemoryMapping_Failed("Could not unlink temporary file");
             }
 
          ~TemporaryFile()
             {
-            delete[] filepath;
+            /*
+            * We can safely close here, because post-mmap the file
+            * will continue to exist until the mmap is unmapped from
+            * our address space upon deallocation.
+            */
             if(fd != -1 && ::close(fd) == -1)
                throw MemoryMapping_Failed("Could not close file");
             }
       private:
          int fd;
-         char* filepath;
       };
 
    TemporaryFile file("/tmp/botan_");
@@ -74,9 +84,6 @@ void* MemoryMapping_Allocator::alloc_block(u32bit n)
    if(file.get_fd() == -1)
       throw MemoryMapping_Failed("Could not create file");
 
-   if(::unlink(file.path().c_str()))
-      throw MemoryMapping_Failed("Could not unlink file '" + file.path() + "'");
-
    if(::lseek(file.get_fd(), n-1, SEEK_SET) < 0)
       throw MemoryMapping_Failed("Could not seek file");