Page MenuHomePhabricator

[compiler-rt] Allow suppression file paths be relative to the location of the executable.
ClosedPublic

Authored by zaks.anna on Jan 30 2015, 5:05 PM.

Details

Summary

The ASanified executable could be launched from different locations. When we cannot find the suppression file relative to the current directory, try to see if the specified path is relative to the location of the executable.

Diff Detail

Event Timeline

zaks.anna updated this revision to Diff 19074.Jan 30 2015, 5:05 PM
zaks.anna retitled this revision from to [compiler-rt] Allow suppression file paths be relative to the location of the executable..
zaks.anna updated this object.
zaks.anna edited the test plan for this revision. (Show Details)
zaks.anna added reviewers: kubamracek, kcc, samsonov, glider.
zaks.anna added a subscriber: Unknown Object (MLST).
kcc edited edge metadata.Feb 2 2015, 6:41 PM

Alexey, any comments?
We already pick up the default blacklist from the compiler distribution at compile time.
The default suppressions are better to be similar.

lib/sanitizer_common/sanitizer_suppressions.cc
87

this is probably unfriendly to windows.

113

ditto

samsonov edited edge metadata.Feb 4 2015, 5:47 PM

Thanks for working on this!

kcc@: We don't have file with default suppressions, do we?

lib/sanitizer_common/sanitizer_mac.cc
216

This can cause memory corruption if the length of the result is larger than buf_len. If you intend to call realpath(), you should probably store the result in the pre-allocated buffer of size at least kMaxPathLength, and then internal_strncpy it into buf.
Alternatively, assert that buf_len is large enough.

lib/sanitizer_common/sanitizer_suppressions.cc
83

Please make the output parameter last.

87

+1. Looks like we need a named constant for path separator that would be different on POSIX/Windows. It can also be used in functions like StripPathPrefix or StripModuleName.

129

You might add a VPrintf() that would tell the actual path of the file with suppressions used by the runtime.

kcc added a comment.Feb 4 2015, 6:11 PM

kcc@: We don't have file with default suppressions, do we?

We do not, but I would rather prefer to have default suppressions than to work like suggested here.

In D7309#118880, @kcc wrote:

kcc@: We don't have file with default suppressions, do we?

We do not, but I would rather prefer to have default suppressions than to work like suggested here.

I assume that the default suppressions are the bugs in libraries we already know about, correct?
My understanding is that those are currently baked into the ASan library.

This workflow is needed for cases where a user runs into an issue in a third party library, which they want to suppress. Allowing them to add the suppressions file can unblock them immediately.

kcc added a comment.Feb 4 2015, 6:59 PM

me being stupid... For some reason I thought you are trying to read suppressions from a path relative to the compiler tool chain.
Ignore my previous comments. :)

zaks.anna updated this revision to Diff 19962.Feb 13 2015, 11:29 PM
zaks.anna edited edge metadata.
  • Made this friendly to Windows. (Introduced "IsPathSeparator()" instead of a constant since multiple characters can serve as separators on Windows.)
  • Addressed all other comments.
  • Included a testcase, which I forgot to add the first time around.
samsonov added inline comments.Feb 17 2015, 5:03 PM
lib/sanitizer_common/sanitizer_mac.cc
217

You can merge these conditions with &&

lib/sanitizer_common/sanitizer_suppressions.cc
83

Function name looks confusing: it return an absolute path, not path relative to executable.

86
if (!ReadBinaryName(exec.data(), exec.size()))
  return false;
...
return true;
89

You can have a buffer overflow on rel_file_path here. Consider taking a length of rel_file_path buffer as an input argument and/or using internal_strncat.

108

const

lib/sanitizer_common/sanitizer_win.cc
320

return c == '\\';

zaks.anna updated this revision to Diff 20225.Feb 18 2015, 2:45 PM

Updated to address the review comments from Alexey.

samsonov accepted this revision.Feb 18 2015, 3:45 PM
samsonov edited edge metadata.

LGTM after addressing a few comments below. Thanks!

lib/sanitizer_common/sanitizer_posix.cc
301

return path != nullptr && IsPathSeparator(path[0])

lib/sanitizer_common/sanitizer_suppressions.cc
90

Max(path_to_exec_len, new_file_path_size - 1);

107

Move these declarations closer to the place where they are actually used.

127

should this be file_path?

This revision is now accepted and ready to land.Feb 18 2015, 3:45 PM
glider edited edge metadata.Feb 19 2015, 7:13 AM

Looks good in general

lib/sanitizer_common/sanitizer_common.h
221

Nit: how about "GetBinaryName"? After all, you're not reading it on Mac.

Comments addressed and committed in r230723.

lib/sanitizer_common/sanitizer_common.h
221

The function only gets the file on Mac, but not on other platforms, so I don't think it should be renamed. (I've moved this from sanitizer_linux.h)

zaks.anna closed this revision.Jun 25 2015, 6:17 PM