Skip to content

[Enhancement] check tuple id is in Descriptor table#53457

Merged
kangkaisen merged 1 commit intoStarRocks:mainfrom
before-Sunrise:tuple_descriptor_defence
Dec 5, 2024
Merged

[Enhancement] check tuple id is in Descriptor table#53457
kangkaisen merged 1 commit intoStarRocks:mainfrom
before-Sunrise:tuple_descriptor_defence

Conversation

@before-Sunrise
Copy link
Contributor

@before-Sunrise before-Sunrise commented Dec 3, 2024

Why I'm doing:

crash like below is because tuple id is not in Descriptor table, so nullptr will be return, and visit nullptr cause crash

@ 0x2d66a6b starrocks::RowDescriptor::init_tuple_idx_map()

 @ 0x2d66cc5 starrocks::RowDescriptor::RowDescriptor()

 @ 0x34ad786 starrocks::ExecNode::ExecNode()

 @ 0x36aa444 starrocks::AssertNumRowsNode::AssertNumRowsNode()

 @ 0x34addbd starrocks::ExecNode::create_vectorized_node()

 @ 0x34aee16 starrocks::ExecNode::create_tree_helper()

 @ 0x34af0fd starrocks::ExecNode::create_tree()

 @ 0x37c8bc2 starrocks::pipeline::FragmentExecutor::_prepare_exec_plan()

 @ 0x37ce80d starrocks::pipeline::FragmentExecutor::prepare()

 @ 0x591422b starrocks::PInternalServiceImplBase<>::_exec_plan_fragment_by_pipeline()

 @ 0x591b819 starrocks::PInternalServiceImplBase<>::_exec_plan_fragment()

 @ 0x5926ae5 starrocks::PInternalServiceImplBase<>::_exec_plan_fragment()

 @ 0x2cb460d starrocks::PriorityThreadPool::work_thread()

What I'm doing:

check before create node in BE's prepare phase

What type of PR is this:

  • BugFix
  • Feature
  • Enhancement
  • Refactor
  • UT
  • Doc
  • Tool

Does this PR entail a change in behavior?

  • Yes, this PR will result in a change in behavior.
  • No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

  • Interface/UI changes: syntax, type conversion, expression evaluation, display information
  • Parameter changes: default values, similar parameters but with different default values
  • Policy changes: use new policy to replace old one, functionality automatically enabled
  • Feature removed
  • Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

  • I have added test cases for my bug fix or my new feature
  • This pr needs user documentation (for new or modified features or behaviors)
  • I have added documentation for my new feature or new function
  • This is a backport pr

Bugfix cherry-pick branch check:

  • I have checked the version labels which the pr will be auto-backported to the target branch
    • 3.4
    • 3.3
    • 3.2
    • 3.1
    • 3.0

@before-Sunrise
fix
9d3eee4 
Signed-off-by: before-Sunrise <[email protected]>
@before-Sunrise before-Sunrise requested a review from a team as a code owner December 3, 2024 03:09
starrocks-cr[bot]
starrocks-cr bot reviewed Dec 3, 2024
View reviewed changes
be/src/exec/exec_node.cpp

void ExecNode::debug_string(int indentation_level, std::stringstream* out) const {
*out << " conjuncts=" << Expr::debug_string(_conjuncts);
*out << " id=" << _id;
Copy link

@starrocks-cr starrocks-cr bot Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most risky bug in this code is:
Failure to log an error may lead to incomplete diagnostics info if exceptions occur.

You can modify the code like this:

Status ExecNode::checkTupleIdsInDescs(const DescriptorTbl& descs, const TPlanNode& planNode) {
    for (auto id : planNode.row_tuples) {
        if (descs.get_tuple_descriptor(id) == nullptr) {
            std::stringstream ss;
            ss << "Plan node id: " << planNode.node_id << ", Tuple ids: ";
            for (auto inner_id : planNode.row_tuples) {  // Use distinct var name to avoid shadowing
                ss << inner_id << ", ";
            }
            LOG(ERROR) << ss.str();
            ss.str("");
            ss << "DescriptorTbl: " << descs.debug_string();
            LOG(ERROR) << ss.str();
            ss.str("");
            ss << "TPlanNode: " << apache::thrift::ThriftDebugString(planNode);
            LOG(ERROR) << ss.str();
            return Status::InternalError("Tuple ids are not in descs");
        }
    }
    return Status::OK();
}

The change ensures that the variable used within the inner loop is distinct (inner_id), helping to prevent subtle bugs related to variable shadowing and ensuring proper logging.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2024

[FE Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2024

[Java-Extensions Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2024

[BE Incremental Coverage Report]

❌ fail : 5 / 18 (27.78%)

file detail

path covered_line new_line coverage not_covered_line_detail
🔵 be/src/exec/exec_node.cpp 5 18 27.78% [589, 590, 591, 592, 594, 595, 596, 597, 598, 599, 600, 601, 602]

@kangkaisen kangkaisen merged commit 7083dfd into StarRocks:main Dec 5, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2024

@Mergifyio backport branch-3.4

@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2024

@Mergifyio backport branch-3.3

@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2024

@Mergifyio backport branch-3.2

@github-actions github-actions bot removed the 3.2 label Dec 5, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2024

@Mergifyio backport branch-3.1

@github-actions github-actions bot removed the 3.1 label Dec 5, 2024
@mergify
Copy link
Contributor

mergify bot commented Dec 5, 2024 •
edited
Loading

backport branch-3.4

✅ Backports have been created

Details

@mergify
Copy link
Contributor

mergify bot commented Dec 5, 2024 •
edited
Loading

backport branch-3.3

✅ Backports have been created

Details

@mergify
Copy link
Contributor

mergify bot commented Dec 5, 2024 •
edited
Loading

backport branch-3.2

✅ Backports have been created

Details

@mergify
Copy link
Contributor

mergify bot commented Dec 5, 2024 •
edited
Loading

backport branch-3.1

✅ Backports have been created

Details

mergify bot pushed a commit that referenced this pull request Dec 5, 2024
@before-Sunrise @mergify
[Enhancement] check tuple id is in Descriptor table (#53457)
9bf5e47 
Signed-off-by: before-Sunrise <[email protected]>
(cherry picked from commit 7083dfd)
@mergify mergify bot mentioned this pull request Dec 5, 2024
Merged
42 tasks
mergify bot pushed a commit that referenced this pull request Dec 5, 2024
@before-Sunrise @mergify
[Enhancement] check tuple id is in Descriptor table (#53457)
62db8bd 
Signed-off-by: before-Sunrise <[email protected]>
(cherry picked from commit 7083dfd)
@mergify mergify bot mentioned this pull request Dec 5, 2024
Merged
42 tasks
mergify bot pushed a commit that referenced this pull request Dec 5, 2024
@before-Sunrise @mergify
[Enhancement] check tuple id is in Descriptor table (#53457)
d160c75 
Signed-off-by: before-Sunrise <[email protected]>
(cherry picked from commit 7083dfd)
@mergify mergify bot mentioned this pull request Dec 5, 2024
Merged
42 tasks
mergify bot pushed a commit that referenced this pull request Dec 5, 2024
@before-Sunrise @mergify
[Enhancement] check tuple id is in Descriptor table (#53457)
5f20991 
Signed-off-by: before-Sunrise <[email protected]>
(cherry picked from commit 7083dfd)
@mergify mergify bot mentioned this pull request Dec 5, 2024
Merged
42 tasks
wanpengfei-git pushed a commit that referenced this pull request Dec 5, 2024
@mergify @before-Sunrise
[Enhancement] check tuple id is in Descriptor table (backport #53457) (…
306364f 
…#53577)

Co-authored-by: before-Sunrise <[email protected]>
wanpengfei-git pushed a commit that referenced this pull request Dec 5, 2024
@mergify @before-Sunrise
[Enhancement] check tuple id is in Descriptor table (backport #53457) (…
5db74e2 
…#53578)

Co-authored-by: before-Sunrise <[email protected]>
wanpengfei-git pushed a commit that referenced this pull request Dec 5, 2024
@mergify @before-Sunrise
[Enhancement] check tuple id is in Descriptor table (backport #53457) (…
f02ca11 
…#53579)

Co-authored-by: before-Sunrise <[email protected]>
wanpengfei-git pushed a commit that referenced this pull request Dec 5, 2024
@mergify @before-Sunrise
[Enhancement] check tuple id is in Descriptor table (backport #53457) (…
93f9805 
…#53580)

Co-authored-by: before-Sunrise <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@satanson satanson satanson approved these changes

@Seaven Seaven Seaven approved these changes

@kangkaisen kangkaisen kangkaisen approved these changes

+1 more reviewer

@starrocks-cr starrocks-cr[bot] starrocks-cr[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@before-Sunrise before-Sunrise

Labels

3.1-merged 3.2-merged 3.3-merged 3.4-merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@before-Sunrise @satanson @Seaven @kangkaisen