Fix warning about the use of mktemp and make platform agnostic by adding and using PipeBase::CreateWithUniqueName.

[flackr]

Summary

Fixes the following warning:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:791: warning: the use of mktemp' is dangerous, better use mkstemp' or `mkdtemp'

mktemp is used to generate a path for creating a named pipe over which the gdb socket is negotiated with the launched process. Since pipes on windows do not exist in filesystem paths, and the mkstemp function is unsuitable given it creates a file at that path, a method PipeBase::CreateWithUniqueName is added which will find a usable name and create a pipe with that name returning the generated name in the passed SmallVectorImpl<char>&.

[flackr]

Just a note, I haven't had a chance to test the new function (or even compile) on Windows yet. I'm getting the environment up on a machine so I can make sure I haven't broken anything there.

[zturner]

include/lldb/Host/posix/PipePosix.h

83

I think this would be better as an std::string. The reason being that this means PipePosix is now going to take a huge amount of stack space, and it's not uncommon for pipes to be allocated on the stack. Since the Pipe isn't used in any performance intensive code anyway, the extra heap allocation isn't really a big deal and it reduces the size of this class by a lot.

source/Host/windows/PipeWindows.cpp

97–107

I would rather use UuidCreate [https://msdn.microsoft.com/en-us/library/windows/desktop/aa379205%28v=vs.85%29.aspx] and then UuidToString [https://msdn.microsoft.com/en-us/library/windows/desktop/aa379352(v=vs.85).aspx]. But if this compiles, and you don't feel comfortable / aren't able to make this change and test it on Windows, I can probably do that.

source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp

785

Should this be strncpy? I feel like we should just be appending the argument directly, regardless of how long it is. So nuke this whole named_pipe_path variable, and instead just call AppendArgument(port_named_pipe.GetName())

[flackr]

Addressed review comments, namely storing a std::string for the m_name member of Pipe(Posix|Windows), using UuidCreate to generate the random pipe name in PipeWindows::CreateWithUniqueName, and referencing the name directly in GDBRemoteCommunication.cpp.

[zturner]

Hi Robert, by any chance did you actually try compiling this on Windows?
The reason I ask is that it just occurred to me that this will now require
linking in rpcrt4.lib, which I don't think happens by default. So this
will cause linker errors. If you tested it and it compiled, then no
problem. But if you didn't test it, then I think it's better to just go
back to the way it was before, and I will do the UuidToString stuff later.

[flackr]

In D7348#118521, @zturner wrote:

Hi Robert, by any chance did you actually try compiling this on Windows?
The reason I ask is that it just occurred to me that this will now require
linking in rpcrt4.lib, which I don't think happens by default. So this
will cause linker errors. If you tested it and it compiled, then no
problem. But if you didn't test it, then I think it's better to just go
back to the way it was before, and I will do the UuidToString stuff later.

I did, but it didn't get as far as the linking phase due to an unrelated error:

D:\src\ll\llvm\tools\lldb\source\Target\ProcessLaunchInfo.cpp(347) : error C2065: 'O_CLOEXEC' : undeclared identifier
[125/270] Building CXX object tools\ll...iles\lldbTarget.dir\ThreadPlan.cpp.obj
ninja: build stopped: subcommand failed.

So I'm not sure if there will be linker errors. I can go back to the way it was before on this.

include/lldb/Host/posix/PipePosix.h

83

Makes sense, thanks. I've done this in the latest patch, although an alternate idea would be to add the generated name as an out parameter of CreateWithUniqueName avoiding storing it as a member at all.

source/Host/windows/PipeWindows.cpp

97–107

Done, thanks for the pointers! I got a Windows machine up and building and was able to verify that my changes in the latest patch compile however it looks like the compile on tip of tree is broken again so I haven't been able to test it yet.

source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp

785

Done.

[ovyalov]

Please see my comments.

source/Host/posix/PipePosix.cpp

196

Is it possible to combine all 3 statements of pipe_spec initialization? I mean, sth like this - const llvm::SmallString<PATH_MAX> pipe_spec (prefix + ".%%%%%%") ;

236

For the sake of consistency, please set the name within PipePosix::OpenAsWriterWithTimeout as well - before m_fds[WRITE] = fd;

[flackr]

Updated PipeBase::CreateWithUniqueName to take a SmallVectorImpl<char>& out parameter for the name rather than storing it in the pipe class, reverted the windows pipe name generation since we don't link against the necessary libs yet, and addressed style comments.

Tested on Linux, compiles on Linux and Windows.

[flackr]

source/Host/posix/PipePosix.cpp

196

Ish:
llvm::SmallString<PATH_MAX> pipe_spec((prefix + ".%%%%%%").str());
Done.

236

No longer relevant, name is now returned as out param in CreateWithUniqueName.

[zturner]

lgtm

[ovyalov]

Minor comment.

source/Host/posix/PipePosix.cpp

216

It seems not needed here or as another option - call clear() unconditionally at the beginning of the method.

[flackr]

Removed unnecessary clear before setting name.

[flackr]

Can one of you land this for me? Thanks.

source/Host/posix/PipePosix.cpp

216

Right, done.

[ovyalov]

Submitted.

AFFECTED FILES

/lldb/trunk/include/lldb/Host/PipeBase.h
/lldb/trunk/include/lldb/Host/posix/PipePosix.h
/lldb/trunk/include/lldb/Host/windows/PipeWindows.h
/lldb/trunk/source/Host/posix/PipePosix.cpp
/lldb/trunk/source/Host/windows/PipeWindows.cpp
/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp

http://reviews.llvm.org/rL228307

[amccarth]

source/Host/windows/PipeWindows.cpp

105

This instance of error hides the one at function scope, so if creating the pipe fails for a reason other then ERROR_ALREADY_EXISTS, this function will return success.

I'll fix this as I switch it to use a GUID on Windows

[flackr]

Committed in http://reviews.llvm.org/rL228307

include/lldb/Host/PipeBase.h

 #include <string>
 
 #include "lldb/Core/Error.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 
 namespace lldb_private

 
     virtual Error CreateNew(bool child_process_inherit) = 0;
     virtual Error CreateNew(llvm::StringRef name, bool child_process_inherit) = 0;
+    virtual Error CreateWithUniqueName(llvm::StringRef prefix, bool child_process_inherit, llvm::SmallVectorImpl<char>& name) = 0;
 
     virtual Error OpenAsReader(llvm::StringRef name, bool child_process_inherit) = 0;
 

include/lldb/Host/posix/PipePosix.h

     Error
     CreateNew(llvm::StringRef name, bool child_process_inherit) override;
     Error
+    CreateWithUniqueName(llvm::StringRef prefix, bool child_process_inherit, llvm::SmallVectorImpl<char>& name) override;
+    Error
     OpenAsReader(llvm::StringRef name, bool child_process_inherit) override;
     Error
     OpenAsWriterWithTimeout(llvm::StringRef name, bool child_process_inherit, const std::chrono::microseconds &timeout) override;

include/lldb/Host/windows/PipeWindows.h

 
     Error CreateNew(bool child_process_inherit) override;
     Error CreateNew(llvm::StringRef name, bool child_process_inherit) override;
+    Error CreateWithUniqueName(llvm::StringRef prefix, bool child_process_inherit, llvm::SmallVectorImpl<char>& name) override;
     Error OpenAsReader(llvm::StringRef name, bool child_process_inherit) override;
     Error OpenAsWriterWithTimeout(llvm::StringRef name, bool child_process_inherit, const std::chrono::microseconds &timeout) override;
 

source/Host/posix/PipePosix.cpp

 
 #include "lldb/Host/posix/PipePosix.h"
 #include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/FileSystem.h"
 
 #include <functional>
 #include <thread>
 
 #include <errno.h>
 #include <fcntl.h>
+#include <limits.h>
 #include <unistd.h>
 #include <sys/types.h>
 #include <sys/stat.h>

 }
 
 Error
+PipePosix::CreateWithUniqueName(llvm::StringRef prefix, bool child_process_inherit, llvm::SmallVectorImpl<char>& name)
+{
+    llvm::SmallString<PATH_MAX> named_pipe_path;
+    llvm::SmallString<PATH_MAX> pipe_spec((prefix + ".%%%%%%").str());
+    FileSpec tmpdir_file_spec;
+    tmpdir_file_spec.Clear();
+    if (HostInfo::GetLLDBPath(ePathTypeLLDBTempSystemDir, tmpdir_file_spec))
+    {
+        tmpdir_file_spec.AppendPathComponent(pipe_spec.c_str());
+    }
+    else
+    {
+        tmpdir_file_spec.AppendPathComponent("/tmp");
+        tmpdir_file_spec.AppendPathComponent(pipe_spec.c_str());
+    }
+
+    // It's possible that another process creates the target path after we've
+    // verified it's available but before we create it, in which case we
+    // should try again.
+    Error error;
+    do {
+        llvm::sys::fs::createUniqueFile(tmpdir_file_spec.GetPath().c_str(), named_pipe_path);
+        error = CreateNew(named_pipe_path, child_process_inherit);
+    } while (error.GetError() == EEXIST);
+
+    if (error.Success())
+        name = named_pipe_path;
+    return error;
+}
+
+Error
 PipePosix::OpenAsReader(llvm::StringRef name, bool child_process_inherit)
 {
     if (CanRead() || CanWrite())

source/Host/windows/PipeWindows.cpp

 
 #include "lldb/Host/windows/PipeWindows.h"
 
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/Process.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include <fcntl.h>

 }
 
 Error
+PipeWindows::CreateWithUniqueName(llvm::StringRef prefix, bool child_process_inherit, llvm::SmallVectorImpl<char>& name)
+{
+    llvm::SmallString<128> pipe_name;
+    Error error;
+    do {
+        pipe_name = prefix;
+        pipe_name += "-";
+        for (unsigned i = 0; i < 6; i++) {
+            pipe_name += "0123456789abcdef"[llvm::sys::Process::GetRandomNumber() & 15];
+        }
+        Error error = CreateNew(pipe_name, child_process_inherit);
+    } while (error.GetError() == ERROR_ALREADY_EXISTS);
+    if (error.Success())
+        name = pipe_name;
+    return error;
+}
+
+Error
 PipeWindows::OpenAsReader(llvm::StringRef name, bool child_process_inherit)
 {
     if (CanRead() || CanWrite())

source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp

 #include "lldb/Host/ThreadLauncher.h"
 #include "lldb/Host/TimeValue.h"
 #include "lldb/Target/Process.h"
+#include "llvm/ADT/SmallString.h"
 
 // Project includes
 #include "ProcessGDBRemoteLog.h"

             debugserver_args.AppendArgument("--setsid");
         }
 
-        char named_pipe_path[PATH_MAX];
-        named_pipe_path[0] = '\0';
+        llvm::SmallString<PATH_MAX> named_pipe_path;
         Pipe port_named_pipe;
 
         bool listen = false;

             {
                 // Binding to port zero, we need to figure out what port it ends up
                 // using using a named pipe...
-                FileSpec tmpdir_file_spec;
-                if (HostInfo::GetLLDBPath(ePathTypeLLDBTempSystemDir, tmpdir_file_spec))
-                {
-                    tmpdir_file_spec.AppendPathComponent("debugserver-named-pipe.XXXXXX");
-                    strncpy(named_pipe_path, tmpdir_file_spec.GetPath().c_str(), sizeof(named_pipe_path));
-                }
-                else
-                {
-                    strncpy(named_pipe_path, "/tmp/debugserver-named-pipe.XXXXXX", sizeof(named_pipe_path));
-                }
-
-                if (::mktemp (named_pipe_path))
-                {
-                    error = port_named_pipe.CreateNew(named_pipe_path, false);
-                    if (error.Fail())
-                        return error;
-                    debugserver_args.AppendArgument("--named-pipe");
-                    debugserver_args.AppendArgument(named_pipe_path);
-                }
+                error = port_named_pipe.CreateWithUniqueName("debugserver-named-pipe", false, named_pipe_path);
+                if (error.Fail())
+                    return error;
+                debugserver_args.AppendArgument("--named-pipe");
+                debugserver_args.AppendArgument(named_pipe_path.c_str());
             }
             else
             {

         
         if (error.Success() && launch_info.GetProcessID() != LLDB_INVALID_PROCESS_ID)
         {
-            if (named_pipe_path[0])
+            if (named_pipe_path.size() > 0)
             {
                 error = port_named_pipe.OpenAsReader(named_pipe_path, false);
                 if (error.Success())

                     else
                     {
                         if (log)
-                            log->Printf("GDBRemoteCommunication::%s() failed to read a port value from named pipe %s: %s", __FUNCTION__, named_pipe_path, error.AsCString());
+                            log->Printf("GDBRemoteCommunication::%s() failed to read a port value from named pipe %s: %s", __FUNCTION__, named_pipe_path.c_str(), error.AsCString());
 
                     }
                     port_named_pipe.Close();

                 else
                 {
                     if (log)
-                        log->Printf("GDBRemoteCommunication::%s() failed to open named pipe %s for reading: %s", __FUNCTION__, named_pipe_path, error.AsCString());
+                        log->Printf("GDBRemoteCommunication::%s() failed to open named pipe %s for reading: %s", __FUNCTION__, named_pipe_path.c_str(), error.AsCString());
                 }
                 const auto err = port_named_pipe.Delete(named_pipe_path);
                 if (err.Fail())
                 {
                     if (log)
-                        log->Printf ("GDBRemoteCommunication::%s failed to delete pipe %s: %s", __FUNCTION__, named_pipe_path, err.AsCString());
+                        log->Printf ("GDBRemoteCommunication::%s failed to delete pipe %s: %s", __FUNCTION__, named_pipe_path.c_str(), err.AsCString());
                 }
             }
             else if (listen)