mirror of
https://github.com/JakeHillion/object-introspection.git
synced 2024-09-18 18:59:04 +01:00
tbv2: fix Thrift isset lookups with padding
Thrift isset was failing with a SEGV if the struct contained padding. This is because we indexed the `isset_indexes` data structure using our field index rather than the index of the field in Thrift. This then gave a rubbish index for any exceeding which happens if we have added padding in the middle of the struct, and this index was looked up in the bitset which can cause a SEGV. Track a new index `thriftFieldIdx` which is only incremented if we've looked up a Thrift index. Namespaced the generated Thrift structs while I was there. This isn't necessary anymore but cleans things up. Test plan: - Added a test case with lots of padding. These don't run in the CI but it passes locally. - `FILTER='OilIntegration.*' make test` - no failures - `FILTER='OidIntegration.*' make test` - no new failures
This commit is contained in:
Jake Hillion
parent
7eebee2bf7
commit
7de35863f5
@ -505,6 +505,7 @@ void CodeGen::getClassSizeFuncConcrete(std::string_view funcName,
|
||||
c.fqName() + ">;\n";
|
||||
}
|
||||
|
||||
size_t thriftFieldIdx = 0;
|
||||
for (size_t i = 0; i < c.members.size(); i++) {
|
||||
const auto& member = c.members[i];
|
||||
if (member.name.starts_with(AddPadding::MemberPrefix))
|
||||
@ -513,8 +514,8 @@ void CodeGen::getClassSizeFuncConcrete(std::string_view funcName,
|
||||
if (thriftIssetMember && thriftIssetMember != &member) {
|
||||
// Capture Thrift's isset value for each field, except for __isset
|
||||
// itself
|
||||
std::string issetIdxStr =
|
||||
"thrift_data::isset_indexes[" + std::to_string(i) + "]";
|
||||
std::string issetIdxStr = "thrift_data::isset_indexes[" +
|
||||
std::to_string(thriftFieldIdx++) + "]";
|
||||
code += " if (&thrift_data::isset_indexes != nullptr && " + issetIdxStr +
|
||||
" != -1) {\n";
|
||||
code += " SAVE_DATA(t." + thriftIssetMember->name + ".get(" +
|
||||
@ -684,6 +685,7 @@ void CodeGen::genClassTraversalFunction(const Class& c, std::string& code) {
|
||||
|
||||
size_t emptySize = code.size();
|
||||
size_t lastNonPaddingElement = getLastNonPaddingMemberIndex(c.members);
|
||||
size_t thriftFieldIdx = 0;
|
||||
for (size_t i = 0; i < lastNonPaddingElement + 1; i++) {
|
||||
const auto& member = c.members[i];
|
||||
if (member.name.starts_with(AddPadding::MemberPrefix)) {
|
||||
@ -696,7 +698,7 @@ void CodeGen::genClassTraversalFunction(const Class& c, std::string& code) {
|
||||
|
||||
if (thriftIssetMember != nullptr && thriftIssetMember != &member) {
|
||||
code += "\n .write(getThriftIsset(t, ";
|
||||
code += std::to_string(i);
|
||||
code += std::to_string(thriftFieldIdx++);
|
||||
code += "))";
|
||||
}
|
||||
|
||||
|
||||
@ -1,4 +1,6 @@
|
||||
thrift_definitions = '''
|
||||
namespace cpp2 ns_thrift_isset
|
||||
|
||||
include "thrift/annotation/cpp.thrift"
|
||||
include "thrift/annotation/thrift.thrift"
|
||||
|
||||
@ -8,6 +10,18 @@ thrift_definitions = '''
|
||||
3: optional i32 c;
|
||||
}
|
||||
|
||||
struct MyThriftStructUnpackedPadded {
|
||||
1: optional i32 a;
|
||||
2: optional i64 b;
|
||||
3: optional i32 c;
|
||||
4: optional i64 d;
|
||||
5: optional i32 e;
|
||||
6: optional i64 f;
|
||||
7: optional i32 g;
|
||||
8: optional i64 h;
|
||||
13: optional i32 i;
|
||||
}
|
||||
|
||||
@cpp.PackIsset
|
||||
struct MyThriftStructPacked {
|
||||
1: optional i32 a;
|
||||
@ -56,7 +70,7 @@ thrift_definitions = '''
|
||||
}
|
||||
'''
|
||||
raw_definitions = '''
|
||||
namespace cpp2 {
|
||||
namespace ns_thrift_isset {
|
||||
MyThriftStructBoxed::MyThriftStructBoxed() :
|
||||
__fbthrift_field_b(),
|
||||
__fbthrift_field_c(),
|
||||
@ -64,7 +78,7 @@ namespace cpp2 {
|
||||
__fbthrift_field_e() {
|
||||
}
|
||||
MyThriftStructBoxed::~MyThriftStructBoxed() {}
|
||||
MyThriftStructBoxed::MyThriftStructBoxed(::cpp2::MyThriftStructBoxed&& other) noexcept :
|
||||
MyThriftStructBoxed::MyThriftStructBoxed(MyThriftStructBoxed&& other) noexcept :
|
||||
__fbthrift_field_a(std::move(other.__fbthrift_field_a)),
|
||||
__fbthrift_field_b(std::move(other.__fbthrift_field_b)),
|
||||
__fbthrift_field_c(std::move(other.__fbthrift_field_c)),
|
||||
@ -72,14 +86,14 @@ namespace cpp2 {
|
||||
__fbthrift_field_e(std::move(other.__fbthrift_field_e)),
|
||||
__isset(other.__isset) {
|
||||
}
|
||||
} // namespace cpp2
|
||||
} // namespace ns_thrift_isset
|
||||
'''
|
||||
|
||||
[cases]
|
||||
[cases.unpacked]
|
||||
param_types = ["const cpp2::MyThriftStructUnpacked&"]
|
||||
param_types = ["const MyThriftStructUnpacked&"]
|
||||
setup = '''
|
||||
cpp2::MyThriftStructUnpacked ret;
|
||||
MyThriftStructUnpacked ret;
|
||||
ret.a_ref() = 1;
|
||||
ret.c_ref() = 1;
|
||||
return ret;
|
||||
@ -106,10 +120,52 @@ namespace cpp2 {
|
||||
]
|
||||
}]'''
|
||||
|
||||
[cases.packed]
|
||||
param_types = ["const cpp2::MyThriftStructPacked&"]
|
||||
[cases.unpacked_padded]
|
||||
param_types = ["const MyThriftStructUnpackedPadded&"]
|
||||
setup = '''
|
||||
cpp2::MyThriftStructPacked ret;
|
||||
MyThriftStructUnpackedPadded ret;
|
||||
ret.a_ref() = 1;
|
||||
ret.c_ref() = 1;
|
||||
return ret;
|
||||
'''
|
||||
features = ["capture-thrift-isset"]
|
||||
expect_json = '''[{
|
||||
"staticSize":80,
|
||||
"dynamicSize":0,
|
||||
"members":[
|
||||
{"name":"__fbthrift_field_a", "staticSize":4, "isset":true},
|
||||
{"name":"__fbthrift_field_b", "staticSize":8, "isset":false},
|
||||
{"name":"__fbthrift_field_c", "staticSize":4, "isset":true},
|
||||
{"name":"__fbthrift_field_d", "staticSize":8, "isset":false},
|
||||
{"name":"__fbthrift_field_e", "staticSize":4, "isset":false},
|
||||
{"name":"__fbthrift_field_f", "staticSize":8, "isset":false},
|
||||
{"name":"__fbthrift_field_g", "staticSize":4, "isset":false},
|
||||
{"name":"__fbthrift_field_h", "staticSize":8, "isset":false},
|
||||
{"name":"__fbthrift_field_i", "staticSize":4, "isset":false},
|
||||
{"name":"__isset", "staticSize":9}
|
||||
]}]'''
|
||||
expect_json_v2 = '''[{
|
||||
"staticSize":80,
|
||||
"exclusiveSize":19,
|
||||
"size":80,
|
||||
"members":[
|
||||
{"name":"__fbthrift_field_a", "staticSize":4, "is_set":true},
|
||||
{"name":"__fbthrift_field_b", "staticSize":8, "is_set":false},
|
||||
{"name":"__fbthrift_field_c", "staticSize":4, "is_set":true},
|
||||
{"name":"__fbthrift_field_d", "staticSize":8, "is_set":false},
|
||||
{"name":"__fbthrift_field_e", "staticSize":4, "is_set":false},
|
||||
{"name":"__fbthrift_field_f", "staticSize":8, "is_set":false},
|
||||
{"name":"__fbthrift_field_g", "staticSize":4, "is_set":false},
|
||||
{"name":"__fbthrift_field_h", "staticSize":8, "is_set":false},
|
||||
{"name":"__fbthrift_field_i", "staticSize":4, "is_set":false},
|
||||
{"name":"__isset", "staticSize":9, "NOT":"is_set"}
|
||||
]
|
||||
}]'''
|
||||
|
||||
[cases.packed]
|
||||
param_types = ["const MyThriftStructPacked&"]
|
||||
setup = '''
|
||||
MyThriftStructPacked ret;
|
||||
ret.a_ref() = 1;
|
||||
ret.c_ref() = 1;
|
||||
ret.d_ref() = 1;
|
||||
@ -154,9 +210,9 @@ namespace cpp2 {
|
||||
]}]'''
|
||||
|
||||
[cases.packed_non_atomic]
|
||||
param_types = ["const cpp2::MyThriftStructPackedNonAtomic&"]
|
||||
param_types = ["const MyThriftStructPackedNonAtomic&"]
|
||||
setup = '''
|
||||
cpp2::MyThriftStructPackedNonAtomic ret;
|
||||
MyThriftStructPackedNonAtomic ret;
|
||||
ret.a_ref() = 1;
|
||||
ret.c_ref() = 1;
|
||||
return ret;
|
||||
@ -185,9 +241,9 @@ namespace cpp2 {
|
||||
]}]'''
|
||||
|
||||
[cases.out_of_order]
|
||||
param_types = ["const cpp2::MyThriftStructOutOfOrder&"]
|
||||
param_types = ["const MyThriftStructOutOfOrder&"]
|
||||
setup = '''
|
||||
cpp2::MyThriftStructOutOfOrder ret;
|
||||
MyThriftStructOutOfOrder ret;
|
||||
ret.b_ref() = 1;
|
||||
return ret;
|
||||
'''
|
||||
@ -213,9 +269,9 @@ namespace cpp2 {
|
||||
]}]'''
|
||||
|
||||
[cases.required]
|
||||
param_types = ["const cpp2::MyThriftStructRequired&"]
|
||||
param_types = ["const MyThriftStructRequired&"]
|
||||
setup = '''
|
||||
cpp2::MyThriftStructRequired ret;
|
||||
MyThriftStructRequired ret;
|
||||
ret.b_ref() = 1;
|
||||
ret.f_ref() = 1;
|
||||
return ret;
|
||||
@ -248,9 +304,9 @@ namespace cpp2 {
|
||||
]}]'''
|
||||
|
||||
[cases.box]
|
||||
param_types = ["const cpp2::MyThriftStructBoxed&"]
|
||||
param_types = ["const MyThriftStructBoxed&"]
|
||||
setup = '''
|
||||
cpp2::MyThriftStructBoxed ret;
|
||||
MyThriftStructBoxed ret;
|
||||
ret.d_ref() = 1;
|
||||
ret.e_ref() = 1;
|
||||
return ret;
|
||||
@ -281,9 +337,9 @@ namespace cpp2 {
|
||||
]}]'''
|
||||
|
||||
[cases.no_capture]
|
||||
param_types = ["const cpp2::MyThriftStructBoxed&"]
|
||||
param_types = ["const MyThriftStructBoxed&"]
|
||||
setup = '''
|
||||
cpp2::MyThriftStructBoxed ret;
|
||||
MyThriftStructBoxed ret;
|
||||
ret.d_ref() = 1;
|
||||
ret.e_ref() = 1;
|
||||
return ret;
|
||||
|
||||
Loading…
Reference in New Issue
Block a user