Fix FileSpec::GetPath to return null-terminated strings

[ki.stfu]

Summary

Before this fix the FileSpec::GetPath() returned string which might be without '\0' at the end.
It could have happened if the size of buffer for path was less than actual path.

Test case:

FileSpec test("/path/to/file", false);
char buf[]="!!!!!!";
test.GetPath(buf, 3);

Before fix:

   233          FileSpec test("/path/to/file", false);
   234          char buf[]="!!!!!!";
   235          test.GetPath(buf, 3);
   236
-> 237          if (core_file)
   238          {
   239              if (!core_file.Exists())
   240              {
(lldb) print buf
(char [7]) $0 = "/pa!!!"

After fix:

   233          FileSpec test("/path/to/file", false);
   234          char buf[]="!!!!!!";
   235          test.GetPath(buf, 3);
   236
-> 237          if (core_file)
   238          {
   239              if (!core_file.Exists())
   240              {
(lldb) print buf
(char [7]) $0 = "/p"

[abidh]

Look good to me. It would be good to fix the similar problem in other places too. One example is SBFileSpec::ResolvePath.

[ki.stfu]

Ok.

[ki.stfu]

Fix strncpy in all files of the "source" folder

[ki.stfu]

@abidh,

Take a look again please.

[zturner]

Please wait to commit this until I've had a chance to review

[zturner]

source/API/SBFileSpec.cpp

97–98 ↗

(On Diff #19742)

I was going to tell you not to use strncpy here, but I guess this is a public API, so we have no choice. I'm really not happy about the signature of these public API methods though. Why does Python care how long it is? Python doesn't have fixed size string buffers, so this restriction makes no sense to me. Anyway, I guess we're stuck with it now.

source/API/SBModule.cpp

209–210 ↗

(On Diff #19742)

Note that this function is currently returning a pointer to stack memory, so it busted in more serious ways than you are currently addressing. Because of that, I will suggest an alternative fix.

214–216 ↗

(On Diff #19742)

Delete these 2 variables and just keep the std::string

221–226 ↗

(On Diff #19742)

I believe this entire block of code can be deleted.

241 ↗

(On Diff #19742)

This is returning a pointer to a stack allocated buffer, which is definitely wrong. Change this to the following:

ConstString const_str(uuid_string.c_str());
return const_str.AsCString();

source/Host/common/SocketAddress.cpp

35–36 ↗

(On Diff #19742)

I would prefer if this function were changed to the following signature:

bool inet_ntop(int af, const void *src, llvm::SmallVector<char> &dst);

38 ↗

(On Diff #19742)

Add

dst.clear();

before this line

49–51 ↗

(On Diff #19742)

With the recommended change to function signature, this becomes:

if (formatted)
{
    dst.append(formatted, formatted+strlen(formatted));
    return true;
}

66 ↗

(On Diff #19742)

Similar.

source/Host/macosx/HostInfoMacOSX.mm

137 ↗

(On Diff #19742)

Bonus points if you can get rid of all the c string manipulation in this function and write everything in terms of llvm::SmallString<> and llvm::StringRef. the basic idea is this:

1. Replace the stack buffer with an llvm::SmallString<PATH_MAX>.
2. use SmallString<>::find() instead of strstr
3. Construct a StringRef() with an explicit length instead of inserting a null terminator
4. Use SmallString<>::replace() instead of strncpy
5. use the file_spec.GetDirectory().SetString(StringRef) instead of file_spec.GetDirectory().SetCString(const char*)

The same improvement could be made to the rest of the functions in this file.

source/Host/posix/HostInfoPosix.cpp

157–158 ↗

(On Diff #19742)

Same could be done here, we shouldn't be using strncpy anywhere in the entire codebase IMO.

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

3643–3644 ↗

(On Diff #19742)

This one can probably stay for now, because we don't have a good alternative to snprintf

[ki.stfu]

source/API/SBModule.cpp

209–210 ↗

(On Diff #19742)

No. It returns a pointer to static memory.

source/Host/common/SocketAddress.cpp

35–36 ↗

(On Diff #19742)

It's a widely known POSIX function. We really want to change its prototype?

[zturner]

source/API/SBModule.cpp

209–210 ↗

(On Diff #19742)

Ahh you're right. Although still not thread safe.

source/Host/common/SocketAddress.cpp

35–36 ↗

(On Diff #19742)

Sure, why not? That's the whole point of the Host layer. To provide a common interface to system calls which differ on different platforms. Posix is just one platform that LLDB supports, and this function is an example of that. As a result, generic code shouldn't be calling inet_ntop() anywhere, because that's a system call whose availability depends on the platform. It should be calling a function from our host layer which calls inet_ntop() on posix, and does other stuff on platforms which don't have inet_ntop.

[clayborg]

Changes requested inline.

source/API/SBFileSpec.cpp

92–98 ↗

(On Diff #19742)

This API is fine. Python has issues with it since it must preallocate a buffer to pass in when used through swig because we do magic on the swig version to make it more usable.

97–98 ↗

(On Diff #19742)

This could also be written as:

snprintf(dst_path, dst_len, "%s", result.c_str());

And it will do the right thing and NULL terminate even during overflow.

source/API/SBModule.cpp

209–241 ↗

(On Diff #19742)

Remove all of this, I just fixed this with:

% svn commit
Sending source/API/SBModule.cpp
Transmitting file data .
Committed revision 228867.

source/Host/common/FileSpec.cpp

785–787

snprintf can be used to do this and NULL terminate correctly. One thing I don't like above strncpy is it will NULL terminate all the way to the end of the string even if the string is shorter, so it does more work than snprintf will.

source/Host/macosx/HostInfoMacOSX.mm

137 ↗

(On Diff #19742)

I would prefer to use std::string instead of llvm::SmallString.

149 ↗

(On Diff #19742)

snprintf

171–172 ↗

(On Diff #19742)

snprintf

193–194 ↗

(On Diff #19742)

snprintf

227–228 ↗

(On Diff #19742)

snprintf

248–249 ↗

(On Diff #19742)

snprintf

source/Host/posix/HostInfoPosix.cpp

157–158 ↗

(On Diff #19742)

Use StringString.Printf() or you can use snprintf if you are putting this into a buffer that is part of the function API

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

3643–3644 ↗

(On Diff #19742)

snprintf

source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp

234–235 ↗

(On Diff #19742)

snprintf

source/lldb.cpp

369–370 ↗

(On Diff #19742)

snprintf

[vharron]

Windows snprintf is nonstandard, I recommend lldb_private::StringStream

[ki.stfu]

Use snprintf instead of strncpy

[tberghammer]

In D7553#122351, @vharron wrote:

Windows snprintf is nonstandard, I recommend lldb_private::StringStream

It is standard since C++11 (not sure about the state of the compiler support)

[zturner]

Windows has it, it just has slightly different behavior.

Anyway, i also recommend StringStream. It's a better, safer wrapper around
the same function and we can encode compiler / platform specific
differences in one location instead of at every callsite.

[ki.stfu]

@zturner, one question about SmallString

source/Host/macosx/HostInfoMacOSX.mm

137 ↗

(On Diff #19742)

RE: 4. Use SmallString<>::replace() instead of strncpy

The replace() is absent. What I can use instead of it?

[ki.stfu]

In D7553#130458, @zturner wrote:

Windows has it, it just has slightly different behavior.

Anyway, i also recommend StringStream. It's a better, safer wrapper around
the same function and we can encode compiler / platform specific
differences in one location instead of at every callsite.

Do you mean StreamString (from source/Core/StreamString.cpp)? How I can get a truncated string in StreamString?

[zturner]

Hmm yea, I'm not sure why I wrote replace(). SmallString::slice() might be
what you want. It returns a StringRef. As long as your program is only
operating on String objects (StringRef, SmallVectorImpl, std::string), you
never have to worry. But if you pass to a function taking a const char* you
need to make ConstString out of it first so it is internalized

[zturner]

I think i missed the context, was just responding from the middle of the
thread. What are you trying to do exactly? (The review doesn't display
nicely on mobile, so it's hard for me to read right now)

[ki.stfu]

In D7553#130463, @zturner wrote:

I think i missed the context, was just responding from the middle of the
thread. What are you trying to do exactly? (The review doesn't display
nicely on mobile, so it's hard for me to read right now)

You and @vharron adviced to use lldb_private::StringStream, but I can't find it in lldb. Instead, I found a StreamString. It was a typo?

And how I can get a truncated string from StreamString? I can do the following but it looks bad:

... prepare StreamString strm ...
std::string str = strm.str();
str.resize(MAX_LEN);
strcpy(dst, str.c_str());

[ki.stfu]

Use llvm::SmallString/llvm::StringRef in HostInfoMacOSX::ComputeSupportExeDirectory;

@clayborg,

Can you check HostInfoMacOSX::ComputeSupportExeDirectory?

[zturner]

source/Host/macosx/HostInfoMacOSX.mm

137–139 ↗

(On Diff #20773)

I know I suggested to use llvm::SmallString and StringRef in a post to the mailing list. However, the outcome of that wasn't a total agreement, so for the time being I would suggest sticking with regular C++ strings. To that end, I would delete the second 2 lines here and just operate on raw_path_str, and adjust the following code accordingly.

[clayborg]

Fix some incorrect usage of snprintf.

source/API/SBFileSpec.cpp

97 ↗

(On Diff #20773)

This should be

::snprintf(dst_path, dst_len, "%s", result.c_str());

snprintf takes the output buffer and output buffer size as the first two arguments. it will also ensure NULL termination even if we go over the buffer size.

source/Host/common/FileSpec.cpp

785–786

this should be:

::snprintf(path, path_max_len, "%s", result.c_str());

source/Host/common/SocketAddress.cpp

51 ↗

(On Diff #20773)

Why did you switch to using an unsafe strcpy() here? You can overflow "dst" no?

66 ↗

(On Diff #20773)

Why did you switch to using an unsafe strcpy() here? You can overflow "dst" no?

source/lldb.cpp

373 ↗

(On Diff #20773)

This should be:

::snprintf(g_version_string, sizeof(g_version_string), "%s", version_string);

[ki.stfu]

@clayborg, see my comments below.

source/Host/common/SocketAddress.cpp

51 ↗

(On Diff #20773)

It can't overflow because we in "if (formatted && strlen(formatted) < size)" branch.

66 ↗

(On Diff #20773)

the same reason.

source/lldb.cpp

373 ↗

(On Diff #20773)

no. we can't do that because it returns a string until first '\n'.

[ki.stfu]

Don't count a result string length when using snprintf; Don't use llvm::SmallString in HostInfoMacOSX::ComputeSupportExeDirectory;

@zturner,

Can you check HostInfoMacOSX::ComputeSupportExeDirectory again?

[ki.stfu]

Use std::string in HostInfoMacOSX instead of char [PATH_MAX]

source/Host/common/FileSpec.cpp

 FileSpec::GetPath(char *path, size_t path_max_len, bool denormalize) const
 {
     if (!path)
         return 0;
 
     std::string result = GetPath(denormalize);
 
     size_t result_length = std::min(path_max_len-1, result.length());
-    ::strncpy(path, result.c_str(), result_length + 1);
+    ::strncpy(path, result.c_str(), result_length);
+    path[result_length] = '\0';
     return result_length;
 }
 
 std::string
 FileSpec::GetPath (bool denormalize) const
 {
     llvm::SmallString<64> result;
     if (m_directory)
         result.append(m_directory.GetCString());