Fix null terminator in Demangle function. #1137
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1137 +/- ##
==========================================
- Coverage 64.03% 60.08% -3.95%
==========================================
Files 20 16 -4
Lines 2580 1794 -786
Branches 888 660 -228
==========================================
- Hits 1652 1078 -574
+ Misses 663 531 -132
+ Partials 265 185 -80
|
|
I added a second commit extending the SymbolizeWithDemangling (just demangle a longer function). It crashes without my fix. |
martonka
force-pushed
the
master
branch
3 times, most recently
from
530d7a7
to
2fe8a07
Compare
If the demangled name is longer than out_size, the null terminator is missing from the output. This will cause a crash in the DemangleInplace() function (symbolize.cc) when calling strlen on the buffer.
|
I had another look at the existing unit tests and now I'm not convinced that trimming the output is a good idea. This will possibly result in some sort of ambiguity or even will confuse users. The easiest way of fixing the issue is to simply return |
| @@ -1350,7 +1351,15 @@ bool Demangle(const char* mangled, char* out, size_t out_size) { | |||
| return false; | |||
| } | |||
|
|
|||
| std::copy_n(unmangled.get(), std::min(n, out_size), out); | |||
| if (out_size > 0) { | |||
There was a problem hiding this comment.
Return false early if n > out_size.
| // n is the size of the allocated buffer, not the length of the string. | ||
| // Therefore, it includes the terminating zero (and possibly additional | ||
| // space). | ||
| std::size_t copy_size = std::min(n, out_size) - 1; |
There was a problem hiding this comment.
All the checks are not necessary after all.
|
@martonka I pushed some changes to your branch. Make sure to pull these before extending the PR. |
If the demangled name is longer than out_size, the null terminator is missing from the output. This will cause a crash in the DemangleInplace() function (symbolize.cc) when calling strlen on the buffer.
Reproduction Steps:
Use a function with a demangled name longer than 256 characters.
Attempt to log a fatal error with a stack trace.
This will cause a crash.
Example input for DemangleInplace that makes it crash (on Ubuntu):
"ZN4kudu9FsManager9AddTenantERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEES8_St8optionalIS6_ESA_SA"