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

95–97

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

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

Add

dst.clear();

before this line

49–51

With the recommended change to function signature, this becomes:

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

66

Similar.

source/Host/macosx/HostInfoMacOSX.mm

136–137

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

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

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

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

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–97

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.

95–97

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

795–797

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

136–137

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

149–150

snprintf

170–172

snprintf

191–193

snprintf

224–226

snprintf

244–247

snprintf

source/Host/posix/HostInfoPosix.cpp

157–158

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

snprintf

source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp

234–235

snprintf

source/lldb.cpp

373–374

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

136–137

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

136–137

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

95–96

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

795–796

this should be:

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

source/Host/common/SocketAddress.cpp

51

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

66

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

source/lldb.cpp

373

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

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

66

the same reason.

source/lldb.cpp

373

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/API/SBFileSpec.cpp

 
 bool
 SBFileSpec::ResolveExecutableLocation ()
 {
     return m_opaque_ap->ResolveExecutableLocation ();
 }
 
 int
 SBFileSpec::ResolvePath (const char *src_path, char *dst_path, size_t dst_len)
 {
     llvm::SmallString<64> result(src_path);
     lldb_private::FileSpec::Resolve (result);
-    size_t result_length = std::min(dst_len-1, result.size());
-    ::strncpy(dst_path, result.c_str(), result_length + 1);
-    return result_length;
+    ::snprintf(dst_path, dst_len, "%s", result.c_str());
+    return std::min(dst_len-1, result.size());
 }
 
 const char *
 SBFileSpec::GetFilename() const
 {
     const char *s = m_opaque_ap->GetFilename().AsCString();
 
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_API));

source/Host/common/FileSpec.cpp

 // values.
 //------------------------------------------------------------------
 size_t
 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);
-    return result_length;
+    ::snprintf(path, path_max_len, "%s", result.c_str());
+    return std::min(path_max_len-1, result.length());
 }
 
 std::string
 FileSpec::GetPath (bool denormalize) const
 {
     llvm::SmallString<64> result;
     if (m_directory)
         result.append(m_directory.GetCString());

source/Host/common/SocketAddress.cpp

 // WindowsXP needs an inet_ntop implementation
 #ifdef _WIN32
 
 #ifndef INET6_ADDRSTRLEN // might not be defined in older Windows SDKs
 #define INET6_ADDRSTRLEN 46
 #endif
 
 // TODO: implement shortened form "::" for runs of zeros
 const char* inet_ntop(int af, const void * src,
                       char * dst, socklen_t size)
 {
     if (size==0)
     {
         return nullptr;
     }
     
     switch (af)
     {
         case AF_INET:
         {
             {
                 const char* formatted = inet_ntoa(*static_cast<const in_addr*>(src));
                 if (formatted && strlen(formatted) < size)
                 {
-                    strncpy(dst, formatted, size);
-                    return dst;
+                    return ::strcpy(dst, formatted);
                 }
             }
             return nullptr;
         case AF_INET6:
             {
                 char tmp[INET6_ADDRSTRLEN] = {0};
                 const uint16_t* src16 = static_cast<const uint16_t*>(src);
                 int full_size = ::snprintf(tmp, sizeof(tmp),
                                           "%x:%x:%x:%x:%x:%x:%x:%x",
                                           ntohs(src16[0]), ntohs(src16[1]), ntohs(src16[2]), ntohs(src16[3]),
                                           ntohs(src16[4]), ntohs(src16[5]), ntohs(src16[6]), ntohs(src16[7])
                                           );
                 if (full_size < static_cast<int>(size))
                 {
-                    strncpy(dst,tmp,size);
-                    return dst;
+                    return ::strcpy(dst, tmp);
                 }
                 return nullptr;
             }
         }
     }
     return nullptr;
 } 
 #endif

source/Host/macosx/HostInfoMacOSX.mm

HostInfoMacOSX::GetProgramFileSpec()
     return g_program_filespec;
 }
 
 bool
 HostInfoMacOSX::ComputeSupportExeDirectory(FileSpec &file_spec)
 {
     FileSpec lldb_file_spec;
     if (!GetLLDBPath(lldb::ePathTypeLLDBShlibDir, lldb_file_spec))
         return false;
-    char raw_path[PATH_MAX];
-    lldb_file_spec.GetPath(raw_path, sizeof(raw_path));
 
-    char *framework_pos = ::strstr(raw_path, "LLDB.framework");
-    if (framework_pos)
+    std::string raw_path = lldb_file_spec.GetPath();
+
+    size_t framework_pos = raw_path.find("LLDB.framework");
+    if (framework_pos != std::string::npos)
     {
         framework_pos += strlen("LLDB.framework");
 #if defined(__arm__) || defined(__arm64__) || defined(__aarch64__)
         // Shallow bundle
-        *framework_pos = '\0';
+        raw_path.resize(framework_pos);
 #else
         // Normal bundle
-        ::strncpy(framework_pos, "/Resources", PATH_MAX - (framework_pos - raw_path));
+        raw_path.resize(framework_pos);
+        raw_path.append("/Resources");
 #endif
     }
-    file_spec.GetDirectory().SetCString(raw_path);
+    file_spec.GetDirectory().SetString(llvm::StringRef(raw_path.c_str(), raw_path.size()));
     return (bool)file_spec.GetDirectory();
 }
 
 bool
 HostInfoMacOSX::ComputeHeaderDirectory(FileSpec &file_spec)
 {
     FileSpec lldb_file_spec;
     if (!HostInfo::GetLLDBPath(lldb::ePathTypeLLDBShlibDir, lldb_file_spec))
         return false;
 
-    char raw_path[PATH_MAX];
-    lldb_file_spec.GetPath(raw_path, sizeof(raw_path));
+    std::string raw_path = lldb_file_spec.GetPath();
 
-    char *framework_pos = ::strstr(raw_path, "LLDB.framework");
-    if (framework_pos)
+    size_t framework_pos = raw_path.find("LLDB.framework");
+    if (framework_pos != std::string::npos)
     {
         framework_pos += strlen("LLDB.framework");
-        ::strncpy(framework_pos, "/Headers", PATH_MAX - (framework_pos - raw_path));
+        raw_path.resize(framework_pos);
+        raw_path.append("/Headers");
     }
-    file_spec.GetDirectory().SetCString(raw_path);
+    file_spec.GetDirectory().SetString(llvm::StringRef(raw_path.c_str(), raw_path.size()));
     return true;
 }
 
 bool
 HostInfoMacOSX::ComputePythonDirectory(FileSpec &file_spec)
 {
 #ifndef LLDB_DISABLE_PYTHON
     FileSpec lldb_file_spec;
     if (!GetLLDBPath(lldb::ePathTypeLLDBShlibDir, lldb_file_spec))
         return false;
 
-    char raw_path[PATH_MAX];
-    lldb_file_spec.GetPath(raw_path, sizeof(raw_path));
+    std::string raw_path = lldb_file_spec.GetPath();
 
-    char *framework_pos = ::strstr(raw_path, "LLDB.framework");
-    if (framework_pos)
+    size_t framework_pos = raw_path.find("LLDB.framework");
+    if (framework_pos != std::string::npos)
     {
         framework_pos += strlen("LLDB.framework");
-        ::strncpy(framework_pos, "/Resources/Python", PATH_MAX - (framework_pos - raw_path));
+        raw_path.resize(framework_pos);
+        raw_path.append("/Resources/Python");
     }
     else
     {
         llvm::SmallString<256> python_version_dir;
         llvm::raw_svector_ostream os(python_version_dir);
         os << "/python" << PY_MAJOR_VERSION << '.' << PY_MINOR_VERSION << "/site-packages";
         os.flush();
 
         // We may get our string truncated. Should we protect this with an assert?
-        ::strncat(raw_path, python_version_dir.c_str(), sizeof(raw_path) - strlen(raw_path) - 1);
+        raw_path.append(python_version_dir.c_str());
     }
-    file_spec.GetDirectory().SetCString(raw_path);
+    file_spec.GetDirectory().SetString(llvm::StringRef(raw_path.c_str(), raw_path.size()));
     return true;
 #else
     return false;
 #endif
 }
 
 bool
 HostInfoMacOSX::ComputeClangDirectory(FileSpec &file_spec)
 {
     FileSpec lldb_file_spec;
     if (!GetLLDBPath (lldb::ePathTypeLLDBShlibDir, lldb_file_spec))
         return false;
     
-    char raw_path[PATH_MAX];
-    lldb_file_spec.GetPath (raw_path, sizeof (raw_path));
+    std::string raw_path = lldb_file_spec.GetPath();
     
-    char *framework_pos = ::strstr (raw_path, "LLDB.framework");
-    if (framework_pos)
+    size_t framework_pos = raw_path.find("LLDB.framework");
+    if (framework_pos != std::string::npos)
     {
         framework_pos += strlen("LLDB.framework");
-        ::strncpy (framework_pos, "/Resources/Clang", PATH_MAX - (framework_pos - raw_path));
+        raw_path.resize(framework_pos);
+        raw_path.append("/Resources/Clang");
     }
-    file_spec.SetFile (raw_path, true);
+    file_spec.SetFile (raw_path.c_str(), true);
     return true;
 }
 
 bool
 HostInfoMacOSX::ComputeSystemPluginsDirectory(FileSpec &file_spec)
 {
     FileSpec lldb_file_spec;
     if (!GetLLDBPath(lldb::ePathTypeLLDBShlibDir, lldb_file_spec))
         return false;
-    char raw_path[PATH_MAX];
-    lldb_file_spec.GetPath(raw_path, sizeof(raw_path));
 
-    char *framework_pos = ::strstr(raw_path, "LLDB.framework");
-    if (!framework_pos)
+    std::string raw_path = lldb_file_spec.GetPath();
+
+    size_t framework_pos = raw_path.find("LLDB.framework");
+    if (framework_pos == std::string::npos)
         return false;
 
     framework_pos += strlen("LLDB.framework");
-    ::strncpy(framework_pos, "/Resources/PlugIns", PATH_MAX - (framework_pos - raw_path));
-    file_spec.GetDirectory().SetCString(raw_path);
+    raw_path.resize(framework_pos);
+    raw_path.append("/Resources/PlugIns");
+    file_spec.GetDirectory().SetString(llvm::StringRef(raw_path.c_str(), raw_path.size()));
     return true;
 }
 
 bool
 HostInfoMacOSX::ComputeUserPluginsDirectory(FileSpec &file_spec)
 {
     FileSpec temp_file("~/Library/Application Support/LLDB/PlugIns", true);
     file_spec.GetDirectory().SetCString(temp_file.GetPath().c_str());

source/Host/posix/HostInfoPosix.cpp

HostInfoPosix::ComputeSupportExeDirectory(FileSpec &file_spec)
     // rooted in entirely different trees.
     if (log)
         log->Printf("HostInfoPosix::ComputeSupportExeDirectory() attempting to derive the bin path (ePathTypeSupportExecutableDir) from "
                     "this path: %s",
                     raw_path);
     char *lib_pos = ::strstr(raw_path, "/lib");
     if (lib_pos != nullptr)
     {
-        // First terminate the raw path at the start of lib.
-        *lib_pos = '\0';
-
         // Now write in bin in place of lib.
-        ::strncpy(lib_pos, "/bin", PATH_MAX - (lib_pos - raw_path));
+        ::snprintf(lib_pos, PATH_MAX - (lib_pos - raw_path), "/bin");
 
         if (log)
             log->Printf("Host::%s() derived the bin path as: %s", __FUNCTION__, raw_path);
     }
     else
     {
         if (log)
             log->Printf("Host::%s() failed to find /lib/liblldb within the shared lib path, bailing on bin path construction",
                         __FUNCTION__);

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

GDBRemoteCommunicationClient::SaveRegisterState (lldb::tid_t tid, uint32_t &save_id)
     {
         const bool thread_suffix_supported = GetThreadSuffixSupported();
         if (thread_suffix_supported || SetCurrentThread(tid))
         {
             char packet[256];
             if (thread_suffix_supported)
                 ::snprintf (packet, sizeof(packet), "QSaveRegisterState;thread:%4.4" PRIx64 ";", tid);
             else
-                ::strncpy (packet, "QSaveRegisterState", sizeof(packet));
+                ::snprintf(packet, sizeof(packet), "QSaveRegisterState");
             
             StringExtractorGDBRemote response;
 
             if (SendPacketAndWaitForResponse(packet, response, false) == PacketResult::Success)
             {
                 if (response.IsUnsupportedResponse())
                 {
                     // This packet isn't supported, don't try calling it again
                     m_supports_QSaveRegisterState = eLazyBoolNo;
                 }

source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp

if (symbol_vendor)
                                                                             value_node = value_node->next;
                                                                         if (value_node && value_node->name)
                                                                         {
                                                                             if (strcmp((const char *)value_node->name, "string") == 0)
                                                                             {
                                                                                 const char *node_content = (const char *)::xmlNodeGetContent(value_node);
                                                                                 if (node_content)
                                                                                 {
-                                                                                    strncpy(DBGBuildSourcePath, node_content, sizeof(DBGBuildSourcePath));
+                                                                                    ::snprintf(DBGBuildSourcePath, sizeof(DBGBuildSourcePath), "%s", node_content);
                                                                                     xmlFree((void *) node_content);
                                                                                 }
                                                                             }
                                                                             key_node = value_node;
                                                                         }
                                                                     }
                                                                     else if (strcmp(key_name, "DBGSourcePath") == 0)
                                                                     {
                                                                         xmlNode *value_node = key_node->next;

source/lldb.cpp

if (g_version_string[0] == '\0')
         
         if (version_string)
             version_string += sizeof("PROJECT:") - 1;
         else
             version_string = "unknown";
         
         const char *newline_loc = strchr(version_string, '\n');
         
-        size_t version_len = sizeof(g_version_string);
+        size_t version_len = sizeof(g_version_string) - 1;
         
         if (newline_loc &&
             (newline_loc - version_string < static_cast<ptrdiff_t>(version_len)))
             version_len = newline_loc - version_string;
         
-        ::strncpy(g_version_string, version_string, version_len);
+        ::snprintf(g_version_string, version_len + 1, "%s", version_string);
     }
 
     return g_version_string;
 #else
     // On Linux/FreeBSD/Windows, report a version number in the same style as the clang tool.
     static std::string g_version_str;
     if (g_version_str.empty())
     {
         g_version_str += "lldb version ";