[Demangler] Improve remangler error handling #39187
Conversation
Mangling can fail, usually because the Node structure has been built incorrectly or because something isn't supported with the old remangler. We shouldn't just terminate the program when that happens, particularly if it happens because someone has passed bad data to the demangler. rdar://79725187
First pass at adding error handling to the actual Remangler. There are still some assert() calls at this point that I'm thinking about. rdar://79725187
Fix the tests to work after the Remangler has been fixed to do error handling. rdar://79725187
This lets us completely remove the unreachable() function from Remangler.cpp. rdar://79725187
First pass at adding error handling to the OldRemangler. Still pondering assert() calls. rdar://79725187
This returns an error code if we're in the runtime, rather than assert()ing. rdar://79725187
Because DEMANGLER_ASSERT() might cause the remanglers to return a ManglingError with the code ManglingError::AssertionFailed, it's useful to have a line number in the ManglingError as well as the other information. This is also potentially helpful for other cases where the code is used multiple times in the remanglers. rdar://79725187
Also fixed a test that broke with the previous commit. rdar://79725187
al45tair
force-pushed
the
problem/79725187
branch
from
ec243b8
to
9013083
Compare
|
@swift-ci Please test |
|
@swift-ci Please smoke test |
|
Build failed |
|
Build failed |
This became an issue because git clang-format reorganised the includes in one of the other files. rdar://79725187
|
@swift-ci Please smoke test |
Defining DEMANGLE_ASSERT as assert() is simple but messes up the file/line information from assert(). Make a dedicated assertion failure function and use that instead to fix. Also enable some tests that were disabled because they triggered abort() calls in the remanglers. rdar://79725187
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
al45tair
force-pushed
the
problem/79725187
branch
from
50cdf1e
to
59cfe35
Compare
The return type of `mangleNode()` is changing to allow the remanglers to report errors to the caller. Change the LLDB plugin to account for that, and try to handle errors more reasonably (previously they would have resulted in program termination). See also apple/swift#39187 rdar://79725187
|
Please test with the following PR: @swift-ci Please smoke test |
|
Please test with the following PR: @swift-ci Please smoke test |
|
Please test with the following PR: @swift-ci Please smoke test |
The return type of `mangleNode()` is changing to allow the remanglers to report errors to the caller. Change the LLDB plugin to account for that, and try to handle errors more reasonably (previously they would have resulted in program termination). See also apple/swift#39187 rdar://79725187
|
Please test with the following PR: @swift-ci Please smoke test |
1 similar comment
|
Please test with the following PR: @swift-ci Please smoke test |
Sed on our Windows builders appears to not support extended regex syntax. Attempt fix by using basic syntax instead. Also, add a '-e'. rdar://79725187
al45tair
force-pushed
the
problem/79725187
branch
from
a64dd04
to
bd31af0
Compare
|
Please test with the following PR: @swift-ci Please smoke test |
I can't see why it's going wrong on Windows, and I don't want to turn the test off just for Windows. Without the "sed", if the line numbers in the remangler files change, this test will fail, which is a bit irritating but I suspect we'll live. rdar://79725187
|
Please test with the following PR: @swift-ci Please smoke test |
|
Please test with the following PR: @swift-ci Please smoke test Linux platform |
There was a problem hiding this comment.
The implementation LGTM.
I'm just wondering why a remangler error could end up in a user facing error (other than caused by mangling bugs)?
Whenever we build a node tree - that's in the demangler and in the runtime - the node tree should be correct. So in theory, there should not be any remangler errors.
So whenever we see the remangler failing, that's caused by a bug which should be fixed in the demangler or in the runtime.
So I'm not sure if this error mechanism is the right way to do it. Why not just adding more asserts in the remangler code?
|
It's mostly a security thing. There are instances (such as in the Objective-C runtime, or in lldb, or, presumably, in the tools the App Store uses to check submitted binaries) where strings from third party programs are demangled and remangled. In some of these cases, the third party may be actively malicious and may be actively trying find a way to cause the demangler to generate a bad node tree. There are also cases where a (valid) newer construct doesn't mangle into the old mangling, or where we're presented with a valid string that would cause a stack overflow if processed. Asserts are fine for the compiler, but in e.g. lldb, which might be passed a bad mangling from somewhere, we don't want to terminate the debugger but would rather display an obvious error message. Likewise, in the runtime, there are sometimes better ways of handling bad data than terminating the process. There's also the issue that assertions are turned off in some builds, which can then lead to crashes or other kinds of undesirable behaviour. |
|
Please test with the following PR: @swift-ci Please smoke test Linux platform |
The return type of `mangleNode()` is changing to allow the remanglers to report errors to the caller. Change the LLDB plugin to account for that, and try to handle errors more reasonably (previously they would have resulted in program termination). See also apple/swift#39187 rdar://79725187
The return type of `mangleNode()` is changing to allow the remanglers to report errors to the caller. Change the LLDB plugin to account for that, and try to handle errors more reasonably (previously they would have resulted in program termination). See also apple/swift#39187 rdar://79725187
|
❤️ |
Adds explicit error handling to the Remangler and OldRemangler, which means that we won't fall flat at runtime when someone feeds us something bad (similarly for
swift-demangle, which shouldn't really just terminate).rdar://79725187