Fix parquet_field_list read_func lambda capture invalid this pointer #16440
Conversation
davidwendt
added
bug
davidwendt
requested review from
ttnghia and
zpuller
and removed request for
a team
|
Thank you for the fix and the detailed writeup @davidwendt. Here is a cookie expressing my gratitude. 🍪 |
| @@ -137,10 +137,10 @@ class parquet_field_bool : public parquet_field { | |||
| struct parquet_field_bool_list : public parquet_field_list<bool, FieldType::BOOLEAN_TRUE> { | |||
| parquet_field_bool_list(int f, std::vector<bool>& v) : parquet_field_list(f, v) | |||
| { | |||
| auto const read_value = [this](uint32_t i, CompactProtocolReader* cpr) { | |||
| auto const read_value = [&val = v](uint32_t i, CompactProtocolReader* cpr) mutable { | |||
There was a problem hiding this comment.
Do you need the mutable keyword? You're capturing by reference so I would have assumed not.
There was a problem hiding this comment.
Exactly, I wanted to ask the same question otherwise LGTM
There was a problem hiding this comment.
Same comment as Yunsong here
There was a problem hiding this comment.
The mutable specifier is defined here: https://en.cppreference.com/w/cpp/language/lambda
mutable Allows body to modify the objects captured by copy, and to call their non-const member functions.
I added the specifier since all of the lambdas call non-const member functions on val.
But I tried compiling it without the specifier and the 12.3 compiler seemed ok so I can technically remove it.
There was a problem hiding this comment.
Since you're capturing val by reference, not by copy, I don't think it's necessary. Not a big deal though.
Description
Fixes internal parquet_field_list subclass constructors capturing invalid this pointer when passing objects to std::make_tuple. The std::make_tuple usage creates a parameter object that is constructed, moved, and destroyed. The this pointer is captured during constructor call. The move constructor is called which creates its own separate this pointer (all member data is moved/copied appropriately). The original this pointer is invalidated by the following destructor. The lambda that was captured in the constructor no longer contains a valid this value in the final moved object.
This PR removes the dependency on the this pointer in the lambda and captures the vector reference instead which is preserved correctly in the object move. The ctor, move, dtor pattern occurs because of how std::make_tuple is implemented by the standard library.
Closes #16408
Checklist