Page MenuHomePhabricator

Fix FileSpec::GetPath to return null-terminated strings
ClosedPublic

Authored by ki.stfu on Feb 11 2015, 3:00 AM.

Details

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"

Diff Detail

Event Timeline

ki.stfu updated this revision to Diff 19739.Feb 11 2015, 3:00 AM
ki.stfu retitled this revision from to Fix FileSpec::GetPath to return null-terminated strings.
ki.stfu updated this object.
ki.stfu edited the test plan for this revision. (Show Details)
ki.stfu added reviewers: abidh, zturner, clayborg.
ki.stfu added subscribers: abidh, zturner, clayborg, Unknown Object (MLST).
abidh accepted this revision.Feb 11 2015, 3:14 AM
abidh edited edge metadata.

Look good to me. It would be good to fix the similar problem in other places too. One example is SBFileSpec::ResolvePath.

This revision is now accepted and ready to land.Feb 11 2015, 3:14 AM
ki.stfu planned changes to this revision.Feb 11 2015, 3:21 AM

Ok.

ki.stfu updated this revision to Diff 19742.Feb 11 2015, 4:03 AM
ki.stfu edited edge metadata.

Fix strncpy in all files of the "source" folder

This revision is now accepted and ready to land.Feb 11 2015, 4:03 AM
ki.stfu requested a review of this revision.Feb 11 2015, 4:05 AM
ki.stfu edited edge metadata.

@abidh,

Take a look again please.

zturner edited edge metadata.Feb 11 2015, 7:25 AM

Please wait to commit this until I've had a chance to review

zturner added inline comments.Feb 11 2015, 10:03 AM
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 added inline comments.Feb 11 2015, 10:29 AM
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 added inline comments.Feb 11 2015, 10:34 AM
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 requested changes to this revision.Feb 11 2015, 11:38 AM
clayborg edited edge metadata.

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

This revision now requires changes to proceed.Feb 11 2015, 11:38 AM

Windows snprintf is nonstandard, I recommend lldb_private::StringStream

ki.stfu updated this revision to Diff 20763.Feb 26 2015, 7:32 AM
ki.stfu edited edge metadata.

Use snprintf instead of strncpy

Windows snprintf is nonstandard, I recommend lldb_private::StringStream

It is standard since C++11 (not sure about the state of the compiler support)

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.

@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?

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?

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

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)

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 updated this revision to Diff 20773.Feb 26 2015, 9:34 AM
ki.stfu edited edge metadata.

Use llvm::SmallString/llvm::StringRef in HostInfoMacOSX::ComputeSupportExeDirectory;

@clayborg,

Can you check HostInfoMacOSX::ComputeSupportExeDirectory?

zturner added inline comments.Feb 26 2015, 9:44 AM
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.

ki.stfu planned changes to this revision.Feb 26 2015, 9:47 AM
clayborg requested changes to this revision.Feb 26 2015, 10:38 AM
clayborg edited edge metadata.

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);

@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 updated this revision to Diff 20833.Feb 26 2015, 11:55 PM
ki.stfu edited edge metadata.

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?

clayborg accepted this revision.Feb 27 2015, 9:36 AM
clayborg edited edge metadata.
This revision is now accepted and ready to land.Feb 27 2015, 9:36 AM
ki.stfu updated this revision to Diff 20876.Feb 27 2015, 11:43 AM
ki.stfu edited edge metadata.

Use std::string in HostInfoMacOSX instead of char [PATH_MAX]

ki.stfu closed this revision.Feb 27 2015, 11:45 AM