Page MenuHomePhabricator

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

Authored by flackr on Feb 2 2015, 8:24 AM.

Details

Reviewers
ovyalov
zturner
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>&.

Diff Detail

Repository
rL LLVM

Event Timeline

flackr updated this revision to Diff 19157.Feb 2 2015, 8:24 AM
flackr retitled this revision from to Fix warning about the use of mktemp and make platform agnostic by adding and using PipeBase::CreateWithUniqueName..
flackr updated this object.
flackr edited the test plan for this revision. (Show Details)
flackr added a reviewer: zturner.
flackr set the repository for this revision to rL LLVM.
flackr added a subscriber: Unknown Object (MLST).

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 added inline comments.Feb 2 2015, 9:38 AM
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 updated this revision to Diff 19332.Feb 4 2015, 10:08 AM

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 edited edge metadata.Feb 4 2015, 10:21 AM

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.

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 added a subscriber: ovyalov.Feb 4 2015, 2:37 PM

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 this revision to Diff 19354.Feb 4 2015, 2:48 PM
flackr updated this object.
flackr edited edge metadata.

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 added inline comments.Feb 4 2015, 2:49 PM
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.

ovyalov accepted this revision.Feb 4 2015, 3:27 PM
ovyalov added a reviewer: 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.

This revision is now accepted and ready to land.Feb 4 2015, 3:27 PM
flackr updated this revision to Diff 19401.Feb 5 2015, 5:28 AM
flackr edited edge metadata.

Removed unnecessary clear before setting name.

flackr added a comment.Feb 5 2015, 5:30 AM

Can one of you land this for me? Thanks.

source/Host/posix/PipePosix.cpp
216

Right, done.

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 added inline comments.
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