You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Thomas Marshall (Code Review)" <ge...@cloudera.org> on 2019/03/13 21:52:14 UTC

[Impala-ASF-CR] IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table 

Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12746


Change subject: IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table
......................................................................

IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table

If HashTable::Init() fails, in some cases we may try to call
HashTable::Close() on the table in GroupingAggregator::Close(), which
can cause a crash.

The solution is to set the hash table to a nullptr if Init() fails.

I also verified that the other use of HashTable, PhjBuilder, handles
this case correctly.

Testing:
- Manually verified that the crash no longer occurs if HashTable::Init
  fails.

Change-Id: I381657c43f93eb4871b93d54532cb886198fd0b2
---
M be/src/exec/grouping-aggregator-partition.cc
1 file changed, 6 insertions(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/12746/1
-- 
To view, visit http://gerrit.cloudera.org:8080/12746
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I381657c43f93eb4871b93d54532cb886198fd0b2
Gerrit-Change-Number: 12746
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>

[Impala-ASF-CR] IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12746 )

Change subject: IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2432/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12746
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I381657c43f93eb4871b93d54532cb886198fd0b2
Gerrit-Change-Number: 12746
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Mar 2019 20:03:57 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12746 )

Change subject: IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table
......................................................................


Patch Set 2: Code-Review+1


-- 
To view, visit http://gerrit.cloudera.org:8080/12746
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I381657c43f93eb4871b93d54532cb886198fd0b2
Gerrit-Change-Number: 12746
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Mar 2019 17:47:50 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12746 )

Change subject: IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/12746/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/12746/2//COMMIT_MSG@14
PS2, Line 14: The patch also fixes a potential issue where HashTable::Init() doesn't
I personally think this is fine - if a function returns an error status, callers should not assume that any out parameters are set unless specifically documented. It doesn't really matter too much in this case, but going too far down this path can lead to a lot of extra defensive code (both in source and in the executable).

I think it's probably a good idea to update the header comments if we're guaranteeing that the out param is set.


http://gerrit.cloudera.org:8080/#/c/12746/2//COMMIT_MSG@21
PS2, Line 21: - Manually verified that the crash no longer occurs if HashTable::Init
Can we write a unit test to exercise this code path, e.g. in hash-table-test (I assume it's hard to trigger in an end-to-end test).


http://gerrit.cloudera.org:8080/#/c/12746/2/be/src/exec/grouping-aggregator-partition.cc
File be/src/exec/grouping-aggregator-partition.cc:

http://gerrit.cloudera.org:8080/#/c/12746/2/be/src/exec/grouping-aggregator-partition.cc@93
PS2, Line 93:   if (!(*got_memory)) {
I think this needs a comment, probably in the header (since it's really a post-condition to the method that hash_tbl is non-NULL only if this method succeeded).



-- 
To view, visit http://gerrit.cloudera.org:8080/12746
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I381657c43f93eb4871b93d54532cb886198fd0b2
Gerrit-Change-Number: 12746
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Mar 2019 17:44:27 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12746 )

Change subject: IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table
......................................................................


Patch Set 4: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/12746
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I381657c43f93eb4871b93d54532cb886198fd0b2
Gerrit-Change-Number: 12746
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Mar 2019 05:15:48 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12746 )

Change subject: IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table
......................................................................

IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table

If HashTable::Init() fails, in some cases we may try to call
HashTable::Close() on the table in GroupingAggregator::Close(), which
can cause a crash.

The solution is to set the hash table to a nullptr if Init() fails.

I also verified that the other use of HashTable, PhjBuilder, handles
this case correctly.

Testing:
- Manually verified that the crash no longer occurs if HashTable::Init
  fails.

Change-Id: I381657c43f93eb4871b93d54532cb886198fd0b2
Reviewed-on: http://gerrit.cloudera.org:8080/12746
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exec/grouping-aggregator-partition.cc
M be/src/exec/grouping-aggregator.h
2 files changed, 8 insertions(+), 1 deletion(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/12746
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I381657c43f93eb4871b93d54532cb886198fd0b2
Gerrit-Change-Number: 12746
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12746 )

Change subject: IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table
......................................................................


Patch Set 4: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/12746
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I381657c43f93eb4871b93d54532cb886198fd0b2
Gerrit-Change-Number: 12746
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Mar 2019 00:50:10 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12746 )

Change subject: IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2429/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12746
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I381657c43f93eb4871b93d54532cb886198fd0b2
Gerrit-Change-Number: 12746
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Mar 2019 00:44:24 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12746 )

Change subject: IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table
......................................................................


Patch Set 4:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3913/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/12746
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I381657c43f93eb4871b93d54532cb886198fd0b2
Gerrit-Change-Number: 12746
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Mar 2019 00:50:11 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12746 )

Change subject: IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2426/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12746
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I381657c43f93eb4871b93d54532cb886198fd0b2
Gerrit-Change-Number: 12746
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Mar 2019 22:25:29 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table

Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tim Armstrong, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/12746

to look at the new patch set (#3).

Change subject: IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table
......................................................................

IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table

If HashTable::Init() fails, in some cases we may try to call
HashTable::Close() on the table in GroupingAggregator::Close(), which
can cause a crash.

The solution is to set the hash table to a nullptr if Init() fails.

I also verified that the other use of HashTable, PhjBuilder, handles
this case correctly.

Testing:
- Manually verified that the crash no longer occurs if HashTable::Init
  fails.

Change-Id: I381657c43f93eb4871b93d54532cb886198fd0b2
---
M be/src/exec/grouping-aggregator-partition.cc
M be/src/exec/grouping-aggregator.h
2 files changed, 8 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/12746/3
-- 
To view, visit http://gerrit.cloudera.org:8080/12746
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I381657c43f93eb4871b93d54532cb886198fd0b2
Gerrit-Change-Number: 12746
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12746 )

Change subject: IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table
......................................................................


Patch Set 3: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/12746
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I381657c43f93eb4871b93d54532cb886198fd0b2
Gerrit-Change-Number: 12746
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Mar 2019 21:53:01 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table

Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tim Armstrong, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/12746

to look at the new patch set (#2).

Change subject: IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table
......................................................................

IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table

If HashTable::Init() fails, in some cases we may try to call
HashTable::Close() on the table in GroupingAggregator::Close(), which
can cause a crash.

The solution is to set the hash table to a nullptr if Init() fails.
The patch also fixes a potential issue where HashTable::Init() doesn't
set its out parameter if Suballocator::Allocate fails.

I also verified that the other use of HashTable, PhjBuilder, handles
this case correctly.

Testing:
- Manually verified that the crash no longer occurs if HashTable::Init
  fails.

Change-Id: I381657c43f93eb4871b93d54532cb886198fd0b2
---
M be/src/exec/grouping-aggregator-partition.cc
M be/src/exec/hash-table.cc
2 files changed, 7 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/12746/2
-- 
To view, visit http://gerrit.cloudera.org:8080/12746
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I381657c43f93eb4871b93d54532cb886198fd0b2
Gerrit-Change-Number: 12746
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>