Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/19087 )
Change subject: IMPALA-11631 Fix impala crashes in impala::TopNNode::Heap::Close() ...................................................................... Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/19087/3/be/src/exec/topn-node.cc File be/src/exec/topn-node.cc: http://gerrit.cloudera.org:8080/#/c/19087/3/be/src/exec/topn-node.cc@696 PS3, Line 696: Otherwise, in case of : // an error, we could double free entries in partition_heap_ in the Close() process. move() will set the original variable to null. So, as we are going through partition_heaps_, the entry.second pointers are being set to null as the Heaps get moved to rematerialized_heaps. My read on the crash is that we try to run Close() on a nullptr Heap. I don't think there is a potential for double free. If I'm understanding this correctly, another way to fix this is for TopNNode::Close() to detect that null pointer and skip calling Close() on entries with a null Heap. i.e. this code: for (auto& entry : partition_heaps_) { entry.second->Close(); } becomes: for (auto& entry : partition_heaps_) { if (entry.second != nullptr) { entry.second->Close(); } } https://github.com/apache/impala/blob/master/be/src/exec/topn-node.cc#L521-L523 -- To view, visit http://gerrit.cloudera.org:8080/19087 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaf45b6ef777f68e1843c076a935e4189acc6990b Gerrit-Change-Number: 19087 Gerrit-PatchSet: 3 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Wed, 05 Oct 2022 00:14:18 +0000 Gerrit-HasComments: Yes
