Fixed buffer overflow in demangler #3624

joewillsher · 2016-07-20T00:57:37Z

What's in this pull request?

Changing the name of ErrorProtocol to Error broke this runtime test — causing a buffer overflow.

The mangled name changed from _TtPs13ErrorProtocol_->_TtPs5Error_ but the strlen didn’t change from 21 to 12.

Error reported is:

=================================================================
==88865==ERROR: AddressSanitizer: global-buffer-overflow on address
0x0001028ba40d at pc 0x000103291a1f bp 0x7fff5d3492c0 sp 0x7fff5d348a80
READ of size 9 at 0x0001028ba40d thread T0
   #0 0x103291a1e in wrap_memmove
(libclang_rt.asan_osx_dynamic.dylib+0x42a1e)
   #1 0x7fff8742e224 in std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> >::__init(char
const*, unsigned long) (libc++.1.dylib+0x3f224)
   #2 0x102907c13 in
swift::Demangle::NodeFactory::create(swift::Demangle::Node::Kind,
llvm::StringRef) string:2044
   #3 0x1028f956b in (anonymous
namespace)::Demangler::demangleTopLevel() Demangle.cpp:358
   #4 0x1028f2e4c in swift::Demangle::demangleSymbolAsNode(char const*,
unsigned long, swift::Demangle::DemangleOptions const&)
Demangle.cpp:2288
   #5 0x1028c425a in swift_reflection_typeRefForMangledTypeName
MetadataReader.h:772
   #6 0x1028b942d in doDumpHeapInstance swift-reflection-test.c:434
   #7 0x1028b9f7a in main swift-reflection-test.c:471
   #8 0x7fff84ab35ac in start (libdyld.dylib+0x35ac)
   #9 0x1  (<unknown module>)

0x0001028ba40d is located 51 bytes to the left of global variable
'<string literal>' defined in
'/Users/buildslave/jenkins/workspace/swift-incremental-asan-RDA/swift/to
ols/swift-reflection-test/swift-reflection-test.c:458:19' (0x1028ba440)
of size 41
 '<string literal>' is ascii string 'swift-reflection-test <binary
filename>
'
0x0001028ba40d is located 0 bytes to the right of global variable
'<string literal>' defined in
'/Users/buildslave/jenkins/workspace/swift-incremental-asan-RDA/swift/to
ols/swift-reflection-test/swift-reflection-test.c:435:15' (0x1028ba400)
of size 13
 '<string literal>' is ascii string '_TtPs5Error_'

SUMMARY: AddressSanitizer: global-buffer-overflow
(libclang_rt.asan_osx_dynamic.dylib+0x42a1e) in wrap_memmove

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

A smoke test on macOS does the following:

Builds the compiler incrementally.
Builds the standard library only for macOS. Simulator standard libraries and
device standard libraries are not built.
lldb is not built.
The test and validation-test targets are run only for macOS. The optimized
version of these tests are not run.

A smoke test on Linux does the following:

Builds the compiler incrementally.
Builds the standard library incrementally.
lldb is build incrementally.
The swift test and validation-test targets are run. The optimized version of these
tests are not run.
lldb is tested.

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

CodaFi · 2016-07-20T16:39:00Z

💥

@swift-ci please test and merge.

rjmccall · 2016-07-20T16:41:23Z

It feels to me that this could and should be done without manually counting the characters in the literal.

CodaFi · 2016-07-20T16:45:52Z

@rjmccall There's an overload of demangleSymbolAsNode for std::string that could be plumbed through instead.

joewillsher · 2016-07-20T16:51:12Z

@CodaFi Does that test what we want here; I could just fixup this test to use strlen instead.

rjmccall · 2016-07-20T17:13:09Z

Or just make a local helper (or expose a static inline C++ function alongside it?) to call the function with an llvm::StringRef.

joewillsher · 2016-07-20T17:30:17Z

@rjmccall There are only 2 callers, and I believe the function needs to be exported to C (so no std::string). I have updated the callers to use strlen instead.

jckarter · 2016-07-20T17:44:25Z

tools/swift-reflection-test/swift-reflection-test.c

          swift_typeref_t ErrorTR
            = swift_reflection_typeRefForMangledTypeName(RC,
-              "_TtPs5Error_", 21);
+              ErrorName, strlen(ErrorName));


@joewillsher strlen is an O(n) operation. Since the string is constant, you can do this to make it a compile-time constant:

static const char ErrorName[] = "_TtPs5Error_"; blahBlah(ErrorName, sizeof(ErrorName) - 1);

It's a test, it's not a big deal. Also, C compilers generally know how to fold strlen of a constant string.

I’ve implemented it as Joe suggested, should I keep it or use strlen?

Using StringRef and passing the string literal as @rjmccall suggested would also do the right thing.

@jckarter this is a c file, isn't StringRef an llvm/c++ type?

Changing the name of ErrorProtocol to Error broke this runtime test — causing a buffer overflow. The mangled name changed from _TtPs13ErrorProtocol_->_TtPs5Error_ but the strlen didn’t change from 21 to 12; I update the callers to use static string length instead of a literal string & length. Error reported is: ================================================================= ==88865==ERROR: AddressSanitizer: global-buffer-overflow on address 0x0001028ba40d at pc 0x000103291a1f bp 0x7fff5d3492c0 sp 0x7fff5d348a80 READ of size 9 at 0x0001028ba40d thread T0 (libclang_rt.asan_osx_dynamic.dylib+0x42a1e) std::__1::char_traits<char>, std::__1::allocator<char> >::__init(char const*, unsigned long) (libc++.1.dylib+0x3f224) swift::Demangle::NodeFactory::create(swift::Demangle::Node::Kind, llvm::StringRef) string:2044 namespace)::Demangler::demangleTopLevel() Demangle.cpp:358 unsigned long, swift::Demangle::DemangleOptions const&) Demangle.cpp:2288 MetadataReader.h:772 0x0001028ba40d is located 51 bytes to the left of global variable '<string literal>' defined in '/Users/buildslave/jenkins/workspace/swift-incremental-asan-RDA/swift/to ols/swift-reflection-test/swift-reflection-test.c:458:19' (0x1028ba440) of size 41 '<string literal>' is ascii string 'swift-reflection-test <binary filename> ' 0x0001028ba40d is located 0 bytes to the right of global variable '<string literal>' defined in '/Users/buildslave/jenkins/workspace/swift-incremental-asan-RDA/swift/to ols/swift-reflection-test/swift-reflection-test.c:435:15' (0x1028ba400) of size 13 '<string literal>' is ascii string '_TtPs5Error_' SUMMARY: AddressSanitizer: global-buffer-overflow (libclang_rt.asan_osx_dynamic.dylib+0x42a1e) in wrap_memmove

CodaFi · 2016-07-20T21:43:26Z

@swift-ci please test and merge.

joewillsher force-pushed the asan branch from 6df16a5 to 6d96822 Compare July 20, 2016 17:25

jckarter reviewed Jul 20, 2016
View reviewed changes

joewillsher force-pushed the asan branch from 6d96822 to d82ad03 Compare July 20, 2016 17:54

swift-ci merged commit 814c3d7 into swiftlang:master Jul 21, 2016

joewillsher deleted the asan branch July 21, 2016 13:52

kateinoigakukun pushed a commit that referenced this pull request Aug 31, 2022

Merge pull request #3624 from swiftwasm/main

d6073fe

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixed buffer overflow in demangler #3624

Fixed buffer overflow in demangler #3624

Uh oh!

joewillsher commented Jul 20, 2016 •

edited

Loading

Uh oh!

CodaFi commented Jul 20, 2016

Uh oh!

rjmccall commented Jul 20, 2016

Uh oh!

CodaFi commented Jul 20, 2016

Uh oh!

joewillsher commented Jul 20, 2016

Uh oh!

rjmccall commented Jul 20, 2016

Uh oh!

joewillsher commented Jul 20, 2016 •

edited

Loading

Uh oh!

jckarter Jul 20, 2016 •

edited

Loading

Uh oh!

rjmccall Jul 20, 2016 •

edited

Loading

Uh oh!

joewillsher Jul 20, 2016 •

edited

Loading

Uh oh!

jckarter Jul 20, 2016

Uh oh!

joewillsher Jul 20, 2016

Uh oh!

CodaFi commented Jul 20, 2016

Uh oh!

Uh oh!

Fixed buffer overflow in demangler #3624

Fixed buffer overflow in demangler #3624

Uh oh!

Conversation

joewillsher commented Jul 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this pull request?

Uh oh!

CodaFi commented Jul 20, 2016

Uh oh!

rjmccall commented Jul 20, 2016

Uh oh!

CodaFi commented Jul 20, 2016

Uh oh!

joewillsher commented Jul 20, 2016

Uh oh!

rjmccall commented Jul 20, 2016

Uh oh!

joewillsher commented Jul 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jckarter Jul 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmccall Jul 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joewillsher Jul 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jckarter Jul 20, 2016

Choose a reason for hiding this comment

Uh oh!

joewillsher Jul 20, 2016

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Jul 20, 2016

Uh oh!

Uh oh!

joewillsher commented Jul 20, 2016 •

edited

Loading

joewillsher commented Jul 20, 2016 •

edited

Loading

jckarter Jul 20, 2016 •

edited

Loading

rjmccall Jul 20, 2016 •

edited

Loading

joewillsher Jul 20, 2016 •

edited

Loading