search

google / glog

C++ implementation of the Google logging module
http://google.github.io/glog/
BSD 3-Clause "New" or "Revised" License
7.1k stars 2.07k forks source link

Fix null terminator in Demangle function. #1137

Open martonka opened 1 month ago

martonka commented 1 month ago

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_ESASA"

google-cla[bot] commented 1 month ago

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-commenter commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 60.08%. Comparing base (2075ae8) to head (f571878).

Additional details and impacted files ```diff @@ 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 ``` | [Files with missing lines](https://app.codecov.io/gh/google/glog/pull/1137?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google) | Coverage Δ | | |---|---|---| | [src/demangle.cc](https://app.codecov.io/gh/google/glog/pull/1137?src=pr&el=tree&filepath=src%2Fdemangle.cc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google#diff-c3JjL2RlbWFuZ2xlLmNj) | `100.00% <ø> (+50.00%)` | :arrow_up: | ... and [12 files with indirect coverage changes](https://app.codecov.io/gh/google/glog/pull/1137/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google)
martonka commented 1 month ago

I added a second commit extending the SymbolizeWithDemangling (just demangle a longer function). It crashes without my fix.

sergiud commented 3 weeks ago

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 false whenever the buffer size is too small to be consistent with the behavior of the existing demangler. In the long term, however, we should switch to dynamic memory (re)allocation (e.g., by using std::string as the return type) to avoid insufficient buffer size situations.

sergiud commented 3 weeks ago

@martonka I pushed some changes to your branch. Make sure to pull these before extending the PR.