[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