Skip to content

Commit a501635

Browse files
mmarchinimmarchini
committed
src: improve Check() reliability
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: - Value classes now have a `valid_` member, and Check() will take this member into account. - A new class, CheckedType, will let us wrap primitive C++ types which were loaded from the paused/crashed process memory, so we can have a Check() method for those values as well to verify if it was possible to load the value successfuly. - Two new macros, RETURN_IF_INVALID and RETURN_IF_THIS_INVALID, to make it easier to return from a function (with a default value) when a object/value was not loaded properly. The goals in the future are: - Replace all uses of Error::Fail as a loading validation tool with RETURN_IF_INVALID, keeping Error::Fail only for unrecoverable errors (instead of the double semantic it has today). - Ensure all methods in llv8.h return either a Value subclass, or a primitive type wrapped in CheckedType - Ensure all calls to methods which will load from the target process memory are followed by RETURN_IF_INVALID. - Ensure all methods in llv8.h start with RETURN_IF_THIS_INVALID. 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). PR-URL: #294 Reviewed-By: Joyee Cheung <[email protected]>
1 parent 0dbfad0 commit a501635

File tree

4 files changed

+68
-32
lines changed

4 files changed

+68
-32
lines changed

‎src/llscan.cc

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -892,11 +892,12 @@ void FindReferencesCmd::ReferenceScanner::PrintRefs(
892892
SBCommandReturnObject& result, v8::String& str, Error& err, int level) {
893893
v8::LLV8* v8 = str.v8();
894894

895-
int64_t repr = str.Representation(err);
895+
v8::CheckedType<int64_t> repr = str.Representation(err);
896+
RETURN_IF_INVALID(repr, );
896897

897898
// Concatenated and sliced strings refer to other strings so
898899
// we need to check their references.
899-
if (repr == v8->string()->kSlicedStringTag) {
900+
if (*repr == v8->string()->kSlicedStringTag) {
900901
v8::SlicedString sliced_str(str);
901902
v8::String parent = sliced_str.Parent(err);
902903
if (err.Success() && parent.raw() == search_value_.raw()) {
@@ -906,7 +907,7 @@ void FindReferencesCmd::ReferenceScanner::PrintRefs(
906907
result.Printf(reference_template.c_str(), str.raw(), type_name.c_str(),
907908
"<Parent>", search_value_.raw());
908909
}
909-
} else if (repr == v8->string()->kConsStringTag) {
910+
} else if (*repr == v8->string()->kConsStringTag) {
910911
v8::ConsString cons_str(str);
911912

912913
v8::String first = cons_str.First(err);
@@ -926,7 +927,7 @@ void FindReferencesCmd::ReferenceScanner::PrintRefs(
926927
result.Printf(reference_template.c_str(), str.raw(), type_name.c_str(),
927928
"<Second>", search_value_.raw());
928929
}
929-
} else if (repr == v8->string()->kThinStringTag) {
930+
} else if (*repr == v8->string()->kThinStringTag) {
930931
v8::ThinString thin_str(str);
931932
v8::String actual = thin_str.Actual(err);
932933
if (err.Success() && actual.raw() == search_value_.raw()) {
@@ -985,12 +986,13 @@ void FindReferencesCmd::ReferenceScanner::ScanRefs(v8::String& str,
985986

986987
v8::LLV8* v8 = str.v8();
987988

988-
int64_t repr = str.Representation(err);
989+
v8::CheckedType<int64_t> repr = str.Representation(err);
990+
RETURN_IF_INVALID(repr, );
989991

990992
// Concatenated and sliced strings refer to other strings so
991993
// we need to check their references.
992994

993-
if (repr == v8->string()->kSlicedStringTag) {
995+
if (*repr == v8->string()->kSlicedStringTag) {
994996
v8::SlicedString sliced_str(str);
995997
v8::String parent = sliced_str.Parent(err);
996998

@@ -999,7 +1001,7 @@ void FindReferencesCmd::ReferenceScanner::ScanRefs(v8::String& str,
9991001
references->push_back(str.raw());
10001002
}
10011003

1002-
} else if (repr == v8->string()->kConsStringTag) {
1004+
} else if (*repr == v8->string()->kConsStringTag) {
10031005
v8::ConsString cons_str(str);
10041006

10051007
v8::String first = cons_str.First(err);
@@ -1013,7 +1015,7 @@ void FindReferencesCmd::ReferenceScanner::ScanRefs(v8::String& str,
10131015
references = llscan_->GetReferencesByValue(second.raw());
10141016
references->push_back(str.raw());
10151017
}
1016-
} else if (repr == v8->string()->kThinStringTag) {
1018+
} else if (*repr == v8->string()->kThinStringTag) {
10171019
v8::ThinString thin_str(str);
10181020
v8::String actual = thin_str.Actual(err);
10191021

@@ -1183,10 +1185,10 @@ void FindReferencesCmd::StringScanner::PrintRefs(SBCommandReturnObject& result,
11831185
// Concatenated and sliced strings refer to other strings so
11841186
// we need to check their references.
11851187

1186-
int64_t repr = str.Representation(err);
1187-
if (err.Fail()) return;
1188+
v8::CheckedType<int64_t> repr = str.Representation(err);
1189+
RETURN_IF_INVALID(repr, );
11881190

1189-
if (repr == v8->string()->kSlicedStringTag) {
1191+
if (*repr == v8->string()->kSlicedStringTag) {
11901192
v8::SlicedString sliced_str(str);
11911193
v8::String parent_str = sliced_str.Parent(err);
11921194
if (err.Fail()) return;
@@ -1197,7 +1199,7 @@ void FindReferencesCmd::StringScanner::PrintRefs(SBCommandReturnObject& result,
11971199
type_name.c_str(), "<Parent>", parent_str.raw(),
11981200
parent.c_str());
11991201
}
1200-
} else if (repr == v8->string()->kConsStringTag) {
1202+
} else if (*repr == v8->string()->kConsStringTag) {
12011203
v8::ConsString cons_str(str);
12021204

12031205
v8::String first_str = cons_str.First(err);
@@ -1313,10 +1315,10 @@ void FindReferencesCmd::StringScanner::ScanRefs(v8::String& str, Error& err) {
13131315
// Concatenated and sliced strings refer to other strings so
13141316
// we need to check their references.
13151317

1316-
int64_t repr = str.Representation(err);
1317-
if (err.Fail()) return;
1318+
v8::CheckedType<int64_t> repr = str.Representation(err);
1319+
RETURN_IF_INVALID(repr, );
13181320

1319-
if (repr == v8->string()->kSlicedStringTag) {
1321+
if (*repr == v8->string()->kSlicedStringTag) {
13201322
v8::SlicedString sliced_str(str);
13211323
v8::String parent_str = sliced_str.Parent(err);
13221324
if (err.Fail()) return;
@@ -1325,7 +1327,7 @@ void FindReferencesCmd::StringScanner::ScanRefs(v8::String& str, Error& err) {
13251327
references = llscan_->GetReferencesByString(parent);
13261328
references->push_back(str.raw());
13271329
}
1328-
} else if (repr == v8->string()->kConsStringTag) {
1330+
} else if (*repr == v8->string()->kConsStringTag) {
13291331
v8::ConsString cons_str(str);
13301332

13311333
v8::String first_str = cons_str.First(err);

‎src/llv8-inl.h

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ inline T LLV8::LoadValue(int64_t addr, Error& err) {
3636

3737

3838
inline bool Smi::Check() const {
39-
return (raw() & v8()->smi()->kTagMask) == v8()->smi()->kTag;
39+
return valid_ && (raw() & v8()->smi()->kTagMask) == v8()->smi()->kTag;
4040
}
4141

4242

@@ -46,7 +46,8 @@ inline int64_t Smi::GetValue() const {
4646

4747

4848
inline bool HeapObject::Check() const {
49-
return (raw() & v8()->heap_obj()->kTagMask) == v8()->heap_obj()->kTag;
49+
return valid_ &&
50+
(raw() & v8()->heap_obj()->kTagMask) == v8()->heap_obj()->kTag;
5051
}
5152

5253

@@ -219,6 +220,7 @@ inline int64_t Map::NumberOfOwnDescriptors(Error& err) {
219220

220221
#define ACCESSOR(CLASS, METHOD, OFF, TYPE) \
221222
inline TYPE CLASS::METHOD(Error& err) { \
223+
if (!Check()) return TYPE(); \
222224
return LoadFieldValue<TYPE>(v8()->OFF, err); \
223225
}
224226

@@ -346,9 +348,11 @@ bool String::IsString(LLV8* v8, HeapObject heap_object, Error& err) {
346348
return type < v8->types()->kFirstNonstringType;
347349
}
348350

349-
inline int64_t String::Representation(Error& err) {
351+
inline CheckedType<int64_t> String::Representation(Error& err) {
352+
RETURN_IF_INVALID((*this), CheckedType<int64_t>());
353+
350354
int64_t type = GetType(err);
351-
if (err.Fail()) return -1;
355+
if (err.Fail()) return CheckedType<int64_t>();
352356
return type & v8()->string()->kRepresentationMask;
353357
}
354358

@@ -640,7 +644,9 @@ inline std::string SlicedString::ToString(Error& err) {
640644
// TODO - Remove when we add support for external strings
641645
// We can't use the offset and length safely if we get "(external)"
642646
// instead of the original parent string.
643-
if (parent.Representation(err) == v8()->string()->kExternalStringTag) {
647+
CheckedType<int64_t> repr = parent.Representation(err);
648+
RETURN_IF_INVALID(repr, std::string());
649+
if (*repr == v8()->string()->kExternalStringTag) {
644650
return parent.ToString(err);
645651
}
646652

‎src/llv8.cc

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -720,13 +720,13 @@ std::string Symbol::ToString(Error& err) {
720720

721721

722722
std::string String::ToString(Error& err) {
723-
int64_t repr = Representation(err);
724-
if (err.Fail()) return std::string();
723+
CheckedType<int64_t> repr = Representation(err);
724+
RETURN_IF_INVALID(repr, std::string());
725725

726726
int64_t encoding = Encoding(err);
727727
if (err.Fail()) return std::string();
728728

729-
if (repr == v8()->string()->kSeqStringTag) {
729+
if (*repr == v8()->string()->kSeqStringTag) {
730730
if (encoding == v8()->string()->kOneByteStringTag) {
731731
OneByteString one(this);
732732
return one.ToString(err);
@@ -739,27 +739,27 @@ std::string String::ToString(Error& err) {
739739
return std::string();
740740
}
741741

742-
if (repr == v8()->string()->kConsStringTag) {
742+
if (*repr == v8()->string()->kConsStringTag) {
743743
ConsString cons(this);
744744
return cons.ToString(err);
745745
}
746746

747-
if (repr == v8()->string()->kSlicedStringTag) {
747+
if (*repr == v8()->string()->kSlicedStringTag) {
748748
SlicedString sliced(this);
749749
return sliced.ToString(err);
750750
}
751751

752752
// TODO(indutny): add support for external strings
753-
if (repr == v8()->string()->kExternalStringTag) {
753+
if (*repr == v8()->string()->kExternalStringTag) {
754754
return std::string("(external)");
755755
}
756756

757-
if (repr == v8()->string()->kThinStringTag) {
757+
if (*repr == v8()->string()->kThinStringTag) {
758758
ThinString thin(this);
759759
return thin.ToString(err);
760760
}
761761

762-
err = Error::Failure("Unsupported string representation %" PRId64, repr);
762+
err = Error::Failure("Unsupported string representation %" PRId64, *repr);
763763
return std::string();
764764
}
765765

‎src/llv8.h

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,14 +37,41 @@ class CodeMap;
3737
NAME(Value* v) : PARENT(v->v8(), v->raw()) {} \
3838
static inline const char* ClassName() { return #NAME; }
3939

40+
#define RETURN_IF_INVALID(var, ret) \
41+
if (!var.Check()) { \
42+
return ret; \
43+
}
44+
45+
#define RETURN_IF_THIS_INVALID(ret) \
46+
if (!this->Check()) { \
47+
return ret; \
48+
}
49+
50+
template <typename T>
51+
class CheckedType {
52+
public:
53+
CheckedType() : valid_(false) {}
54+
CheckedType(T val) : val_(val), valid_(true) {}
55+
56+
T operator*() {
57+
// TODO(mmarchini): Check()
58+
return val_;
59+
}
60+
inline bool Check() const { return valid_; }
61+
62+
private:
63+
T val_;
64+
bool valid_;
65+
};
66+
4067
class Value {
4168
public:
4269
Value(const Value& v) = default;
4370
Value(Value& v) = default;
44-
Value() : v8_(nullptr), raw_(-1) {}
71+
Value() : v8_(nullptr), raw_(-1), valid_(false) {}
4572
Value(LLV8* v8, int64_t raw) : v8_(v8), raw_(raw) {}
4673

47-
inline bool Check() const { return true; }
74+
inline bool Check() const { return valid_; }
4875

4976
inline int64_t raw() const { return raw_; }
5077
inline LLV8* v8() const { return v8_; }
@@ -68,6 +95,7 @@ class Value {
6895
protected:
6996
LLV8* v8_;
7097
int64_t raw_ = 0x0;
98+
bool valid_ = true;
7199
};
72100

73101
class Smi : public Value {
@@ -134,7 +162,7 @@ class String : public HeapObject {
134162
V8_VALUE_DEFAULT_METHODS(String, HeapObject)
135163

136164
inline int64_t Encoding(Error& err);
137-
inline int64_t Representation(Error& err);
165+
inline CheckedType<int64_t> Representation(Error& err);
138166
inline Smi Length(Error& err);
139167

140168
std::string ToString(Error& err);

0 commit comments

Comments
 (0)