[Lldb-commits] [PATCH] Fix FileSpec::GetPath to return null-terminated strings

[Zachary Turner]

================
Comment at: source/API/SBFileSpec.cpp:97-98
@@ -96,3 +96,4 @@
     size_t result_length = std::min(dst_len-1, result.size());
-    ::strncpy(dst_path, result.c_str(), result_length + 1);
+    ::strncpy(dst_path, result.c_str(), result_length);
+    dst_path[result_length] = '\0';
     return result_length;
----------------
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.

================
Comment at: source/API/SBModule.cpp:209-210
@@ -208,4 +208,4 @@
 
 const char *
 SBModule::GetUUIDString () const
 {
----------------
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.

================
Comment at: source/API/SBModule.cpp:214-216
@@ -213,5 +213,5 @@
 
     static char uuid_string_buffer[80];
     const char *uuid_c_string = NULL;
     std::string uuid_string;
     ModuleSP module_sp (GetSP ());
----------------
Delete these 2 variables and just keep the std::string

================
Comment at: source/API/SBModule.cpp:221-226
@@ -220,8 +220,8 @@
 
     if (!uuid_string.empty())
     {
-        strncpy (uuid_string_buffer, uuid_string.c_str(), sizeof (uuid_string_buffer));
+        ::strncpy (uuid_string_buffer, uuid_string.c_str(), sizeof (uuid_string_buffer) - 1);
         uuid_string_buffer[sizeof (uuid_string_buffer) - 1] = '\0';
         uuid_c_string = uuid_string_buffer;
     }
 
----------------
I believe this entire block of code can be deleted.

================
Comment at: source/API/SBModule.cpp:241
@@ -240,3 +240,3 @@
     }
     return uuid_c_string;
 }
----------------
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();

================
Comment at: source/Host/common/SocketAddress.cpp:35-36
@@ -34,4 +34,4 @@
 // TODO: implement shortened form "::" for runs of zeros
 const char* inet_ntop(int af, const void * src,
                       char * dst, socklen_t size)
 {
----------------
I would prefer if this function were changed to the following signature:

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

================
Comment at: source/Host/common/SocketAddress.cpp:38
@@ -37,3 +37,3 @@
 {
     if (size==0)
     {
----------------
Add

    dst.clear();

before this line

================
Comment at: source/Host/common/SocketAddress.cpp:49-51
@@ -48,6 +48,5 @@
                 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);
                 }
----------------
With the recommended change to function signature, this becomes:

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

================
Comment at: source/Host/common/SocketAddress.cpp:66
@@ -66,4 +65,3 @@
                 {
-                    strncpy(dst,tmp,size);
-                    return dst;
+                    return ::strcpy(dst, tmp);
                 }
----------------
Similar.

================
Comment at: source/Host/macosx/HostInfoMacOSX.mm:137
@@ -136,3 +136,3 @@
         return false;
     char raw_path[PATH_MAX];
     lldb_file_spec.GetPath(raw_path, sizeof(raw_path));
----------------
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.

================
Comment at: source/Host/posix/HostInfoPosix.cpp:157-158
@@ -159,3 +156,4 @@
         // Now write in bin in place of lib.
-        ::strncpy(lib_pos, "/bin", PATH_MAX - (lib_pos - raw_path));
+        ::strncpy(lib_pos, "/bin", PATH_MAX - (lib_pos - raw_path) - 1);
+        lib_pos[PATH_MAX - (lib_pos - raw_path) - 1] = '\0';
 
----------------
Same could be done here, we shouldn't be using strncpy anywhere in the entire codebase IMO.

================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3643-3644
@@ +3642,4 @@
+            {
+                ::strncpy (packet, "QSaveRegisterState", sizeof(packet) - 1);
+                packet[sizeof(packet) - 1] = '\0';
+            }
----------------
This one can probably stay for now, because we don't have a good alternative to snprintf

http://reviews.llvm.org/D7553

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/