[Lldb-commits] [PATCH] Fix FileSpec::GetPath to return null-terminated strings
Greg Clayton
clayborg at gmail.com
Wed Feb 11 11:38:54 PST 2015
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/
More information about the lldb-commits
mailing list