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

Vince Harron vharron at google.com
Wed Feb 11 18:26:35 PST 2015


Windows snprintf is nonstandard, I recommend lldb_private::StringStream
On Feb 11, 2015 11:39 AM, "Greg Clayton" <clayborg at gmail.com> wrote:

> Changes requested inline.
>
>
> ================
> Comment at: source/API/SBFileSpec.cpp:92-98
> @@ -91,8 +91,9 @@
>  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);
> +    ::strncpy(dst_path, result.c_str(), result_length);
> +    dst_path[result_length] = '\0';
>      return result_length;
> ----------------
> 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.
>
> ================
> 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;
> ----------------
> zturner wrote:
> > 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.
> 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.
>
> ================
> Comment at: source/API/SBModule.cpp:209-241
> @@ -208,35 +208,35 @@
>
>  const char *
>  SBModule::GetUUIDString () const
>  {
>      Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_API));
>
>      static char uuid_string_buffer[80];
>      const char *uuid_c_string = NULL;
>      std::string uuid_string;
>      ModuleSP module_sp (GetSP ());
>      if (module_sp)
>          uuid_string = module_sp->GetUUID().GetAsString();
>
>      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;
>      }
>
>      if (log)
>      {
>          if (!uuid_string.empty())
>          {
>              StreamString s;
>              module_sp->GetUUID().Dump (&s);
>              log->Printf ("SBModule(%p)::GetUUIDString () => %s",
>                           static_cast<void*>(module_sp.get()),
> s.GetData());
>          }
>          else
>              log->Printf ("SBModule(%p)::GetUUIDString () => NULL",
>                           static_cast<void*>(module_sp.get()));
>      }
>      return uuid_c_string;
>  }
> ----------------
> zturner wrote:
> > ki.stfu wrote:
> > > zturner wrote:
> > > > 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.
> > > No. It returns a pointer to static memory.
> > Ahh you're right.  Although still not thread safe.
> Remove all of this, I just fixed this with:
>
> % svn commit
> Sending        source/API/SBModule.cpp
> Transmitting file data .
> Committed revision 228867.
>
>
> ================
> Comment at: source/Host/common/FileSpec.cpp:785-786
> @@ -784,3 +784,4 @@
>      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;
> ----------------
> 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.
>
> ================
> 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));
> ----------------
> zturner wrote:
> > 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.
> I would prefer to use std::string instead of llvm::SmallString.
>
> ================
> Comment at: source/Host/macosx/HostInfoMacOSX.mm:149
> @@ -148,2 +148,3 @@
>          // Normal bundle
> -        ::strncpy(framework_pos, "/Resources", PATH_MAX - (framework_pos
> - raw_path));
> +        ::strncpy(framework_pos, "/Resources", PATH_MAX - (framework_pos
> - raw_path) - 1);
> +        framework_pos[PATH_MAX - (framework_pos - raw_path) - 1] = '\0';
> ----------------
> snprintf
>
> ================
> Comment at: source/Host/macosx/HostInfoMacOSX.mm:171-172
> @@ -169,3 +170,4 @@
>          framework_pos += strlen("LLDB.framework");
> -        ::strncpy(framework_pos, "/Headers", PATH_MAX - (framework_pos -
> raw_path));
> +        ::strncpy(framework_pos, "/Headers", PATH_MAX - (framework_pos -
> raw_path) - 1);
> +        framework_pos[PATH_MAX - (framework_pos - raw_path) - 1] = '\0';
>      }
> ----------------
> snprintf
>
> ================
> Comment at: source/Host/macosx/HostInfoMacOSX.mm:193-194
> @@ -190,3 +192,4 @@
>          framework_pos += strlen("LLDB.framework");
> -        ::strncpy(framework_pos, "/Resources/Python", PATH_MAX -
> (framework_pos - raw_path));
> +        ::strncpy(framework_pos, "/Resources/Python", PATH_MAX -
> (framework_pos - raw_path) - 1);
> +        framework_pos[PATH_MAX - (framework_pos - raw_path) - 1] = '\0';
>      }
> ----------------
> snprintf
>
> ================
> Comment at: source/Host/macosx/HostInfoMacOSX.mm:227-228
> @@ -223,3 +226,4 @@
>          framework_pos += strlen("LLDB.framework");
> -        ::strncpy (framework_pos, "/Resources/Clang", PATH_MAX -
> (framework_pos - raw_path));
> +        ::strncpy(framework_pos, "/Resources/Clang", PATH_MAX -
> (framework_pos - raw_path) - 1);
> +        framework_pos[PATH_MAX - (framework_pos - raw_path) - 1] = '\0';
>      }
> ----------------
> snprintf
>
> ================
> Comment at: source/Host/macosx/HostInfoMacOSX.mm:248-249
> @@ -243,3 +247,4 @@
>      framework_pos += strlen("LLDB.framework");
> -    ::strncpy(framework_pos, "/Resources/PlugIns", PATH_MAX -
> (framework_pos - raw_path));
> +    ::strncpy(framework_pos, "/Resources/PlugIns", PATH_MAX -
> (framework_pos - raw_path) - 1);
> +    framework_pos[PATH_MAX - (framework_pos - raw_path) - 1] = '\0';
>      file_spec.GetDirectory().SetCString(raw_path);
> ----------------
> snprintf
>
> ================
> 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';
>
> ----------------
> zturner wrote:
> > Same could be done here, we shouldn't be using strncpy anywhere in the
> entire codebase IMO.
> Use StringString.Printf() or you can use snprintf if you are putting this
> into a buffer that is part of the function API
>
> ================
> Comment at:
> source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3643-3644
> @@ +3642,4 @@
> +            {
> +                ::strncpy (packet, "QSaveRegisterState", sizeof(packet) -
> 1);
> +                packet[sizeof(packet) - 1] = '\0';
> +            }
> ----------------
> zturner wrote:
> > This one can probably stay for now, because we don't have a good
> alternative to snprintf
> snprintf
>
> ================
> Comment at:
> source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp:234-235
> @@ -233,3 +233,4 @@
>
>        {
> -
>           strncpy(DBGBuildSourcePath, node_content,
> sizeof(DBGBuildSourcePath));
> +
>           strncpy(DBGBuildSourcePath, node_content,
> sizeof(DBGBuildSourcePath) - 1);
> +
>           DBGBuildSourcePath[sizeof(DBGBuildSourcePath) - 1] = '\0';
>
>            xmlFree((void *) node_content);
> ----------------
> snprintf
>
> ================
> Comment at: source/lldb.cpp:369-370
> @@ -368,3 +368,4 @@
>
>          ::strncpy(g_version_string, version_string, version_len);
> +        g_version_string[version_len] = '\0';
>      }
> ----------------
> snprintf
>
> http://reviews.llvm.org/D7553
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20150211/76a724d9/attachment.html>


More information about the lldb-commits mailing list