nodejs / llnode

An lldb plugin for Node.js and V8, which enables inspection of JavaScript states for insights into Node.js processes and their core dumps.
Other
1.15k stars 99 forks source link

src: improve Check() reliability #294

Closed mmarchini closed 4 years ago

mmarchini commented 4 years ago

This is a first step towards making llnode more reliable. Right now, llnode doesn't allow us to deal with partially-loaded objects (some fields working, some not working). As a postmortem tool, llnode should be able to handle partial loading of objects, as this is the proper way to deal with corrupt memory. If llnode can handle corrupted memory, it will also be able to understand the memory of unsupported Node.js versons, although some fields and objects won't load.

There are two problems regarding reliability in llnode: first, we have several attempts to access fields from the crashed/paused process memory without validating if we have enough information (and correct informaton) to do so. Second, we use Error::Fail() as the primary tool to verify if there was an error loading a field/objects/etc., but Error::Fail usually propagates and will cause us to prematurely stop processing the memory.

This commit introduces a few things to help us improve reliability in the future:

The goals in the future are:

We could make all those changes in a single PR, but it would take a huge amount of work and the PR would be extremely long, making it harder to review. Instead, I suggest we make incremental changes as we see fit, until we achieve the goals described above.

The method of choice to start was String::Representation, because by making this method more robust we fix a crash on Node.js v12 after running v8 bt if there's an optimized function on the stack (the function won't be translated, but it's better than a hard crash).

codecov-io commented 4 years ago

Codecov Report

Merging #294 into master will decrease coverage by 0.38%. The diff coverage is 61.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #294      +/-   ##
==========================================
- Coverage   79.84%   79.45%   -0.39%     
==========================================
  Files          33       33              
  Lines        4232     4216      -16     
==========================================
- Hits         3379     3350      -29     
- Misses        853      866      +13
Impacted Files Coverage Δ
src/llv8.h 97.61% <100%> (-2.39%) :arrow_down:
src/llv8-inl.h 92.58% <100%> (-0.1%) :arrow_down:
src/llscan.cc 61.33% <27.77%> (ø) :arrow_up:
src/llv8.cc 73.98% <87.5%> (-1.49%) :arrow_down:
src/llv8-constants.h 98.48% <0%> (-1.52%) :arrow_down:
src/llv8-constants.cc 82.52% <0%> (-0.98%) :arrow_down:
src/llnode_module.cc 89.87% <0%> (-0.64%) :arrow_down:
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0dbfad0...37b528b. Read the comment docs.

mmarchini commented 4 years ago

Landed in a501635