You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2018/11/21 19:12:13 UTC
[Impala-ASF-CR] IMPALA-7851: fix overflow in ReserveSpace()
Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11976
Change subject: IMPALA-7851: fix overflow in ReserveSpace()
......................................................................
IMPALA-7851: fix overflow in ReserveSpace()
The overflow could result in reserving huge amounts of memory in the
vector, which resulted in various misbehaviour such as hangs.
Change-Id: Iaec944f2149a6b9b605a5a885357fd54754dc046
---
M be/src/service/hs2-util.cc
1 file changed, 15 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/11976/1
--
To view, visit http://gerrit.cloudera.org:8080/11976
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaec944f2149a6b9b605a5a885357fd54754dc046
Gerrit-Change-Number: 11976
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-7851: fix overflow in ReserveSpace()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11976 )
Change subject: IMPALA-7851: fix overflow in ReserveSpace()
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1423/ : 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/11976
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaec944f2149a6b9b605a5a885357fd54754dc046
Gerrit-Change-Number: 11976
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 21 Nov 2018 20:08:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7851: fix underflow in ReserveSpace()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Thomas Marshall, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11976
to look at the new patch set (#4).
Change subject: IMPALA-7851: fix underflow in ReserveSpace()
......................................................................
IMPALA-7851: fix underflow in ReserveSpace()
The (num_rows - start_idx) calculation was incorrect because
num_rows is *not* inclusive of start_idx - it is the number
of rows after start_idx to add to the output batch rather than
one past the last row to add.
(run_rows - start_idx) could then be negative, which results in
an underflow when it is implicitly converted to uint32_t.
The underflow could result in reserving huge amounts of memory in the
vector, which resulted in various misbehaviour such as hangs.
Testing:
Looped the test under ASAN. Before the fix it hung reliably. After it
succeeds quickly.
Change-Id: Iaec944f2149a6b9b605a5a885357fd54754dc046
---
M be/src/service/hs2-util.cc
1 file changed, 15 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/11976/4
--
To view, visit http://gerrit.cloudera.org:8080/11976
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaec944f2149a6b9b605a5a885357fd54754dc046
Gerrit-Change-Number: 11976
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-7851: fix underflow in ReserveSpace()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11976 )
Change subject: IMPALA-7851: fix underflow in ReserveSpace()
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11976/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11976/2//COMMIT_MSG@9
PS2, Line 9: The overflow could result in reserving huge amounts of memory in the
> Could you provide some more context on why the fix works? eg. how does it n
It's really an underflow now that I think about it. Updated the message wiht a better explanation
--
To view, visit http://gerrit.cloudera.org:8080/11976
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaec944f2149a6b9b605a5a885357fd54754dc046
Gerrit-Change-Number: 11976
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Nov 2018 21:45:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7851: fix underflow in ReserveSpace()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11976 )
Change subject: IMPALA-7851: fix underflow in ReserveSpace()
......................................................................
IMPALA-7851: fix underflow in ReserveSpace()
The (num_rows - start_idx) calculation was incorrect because
num_rows is *not* inclusive of start_idx - it is the number
of rows after start_idx to add to the output batch rather than
one past the last row to add.
(run_rows - start_idx) could then be negative, which results in
an underflow when it is implicitly converted to uint32_t.
The underflow could result in reserving huge amounts of memory in the
vector, which resulted in various misbehaviour such as hangs.
Testing:
Looped the test under ASAN. Before the fix it hung reliably. After it
succeeds quickly.
Change-Id: Iaec944f2149a6b9b605a5a885357fd54754dc046
Reviewed-on: http://gerrit.cloudera.org:8080/11976
Reviewed-by: Thomas Marshall <th...@cmu.edu>
Tested-by: Tim Armstrong <ta...@cloudera.com>
---
M be/src/service/hs2-util.cc
1 file changed, 15 insertions(+), 13 deletions(-)
Approvals:
Thomas Marshall: Looks good to me, approved
Tim Armstrong: Verified
--
To view, visit http://gerrit.cloudera.org:8080/11976
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iaec944f2149a6b9b605a5a885357fd54754dc046
Gerrit-Change-Number: 11976
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-7851: fix overflow in ReserveSpace()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11976 )
Change subject: IMPALA-7851: fix overflow in ReserveSpace()
......................................................................
Patch Set 2:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3484/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/11976
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaec944f2149a6b9b605a5a885357fd54754dc046
Gerrit-Change-Number: 11976
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 21 Nov 2018 19:23:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7851: fix underflow in ReserveSpace()
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11976 )
Change subject: IMPALA-7851: fix underflow in ReserveSpace()
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11976
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaec944f2149a6b9b605a5a885357fd54754dc046
Gerrit-Change-Number: 11976
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Nov 2018 21:47:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7851: fix overflow in ReserveSpace()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Thomas Marshall, Joe McDonnell,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11976
to look at the new patch set (#2).
Change subject: IMPALA-7851: fix overflow in ReserveSpace()
......................................................................
IMPALA-7851: fix overflow in ReserveSpace()
The overflow could result in reserving huge amounts of memory in the
vector, which resulted in various misbehaviour such as hangs.
Testing:
Looped the test under ASAN. Before the fix it hung reliably. After it
succeeds quickly.
Change-Id: Iaec944f2149a6b9b605a5a885357fd54754dc046
---
M be/src/service/hs2-util.cc
1 file changed, 15 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/11976/2
--
To view, visit http://gerrit.cloudera.org:8080/11976
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaec944f2149a6b9b605a5a885357fd54754dc046
Gerrit-Change-Number: 11976
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
[Impala-ASF-CR] IMPALA-7851: fix underflow in ReserveSpace()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11976 )
Change subject: IMPALA-7851: fix underflow in ReserveSpace()
......................................................................
Patch Set 4: Verified+1
Carry +2 since it was just a commit message change.
--
To view, visit http://gerrit.cloudera.org:8080/11976
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaec944f2149a6b9b605a5a885357fd54754dc046
Gerrit-Change-Number: 11976
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Nov 2018 23:31:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7851: fix underflow in ReserveSpace()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Thomas Marshall, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11976
to look at the new patch set (#3).
Change subject: IMPALA-7851: fix underflow in ReserveSpace()
......................................................................
IMPALA-7851: fix underflow in ReserveSpace()
The (num_rows - start_idx) calculation was incorrect because
num_rows is *not* inclusive of start_idx - it is the number
of rows after start_idx to add to the output patch rather than
one past the last row to add to the back.
(run_rows - start_idx) could then be negative, which results in
an underflow when it is implicitly converted to uint32_t.
The underflow could result in reserving huge amounts of memory in the
vector, which resulted in various misbehaviour such as hangs.
Testing:
Looped the test under ASAN. Before the fix it hung reliably. After it
succeeds quickly.
Change-Id: Iaec944f2149a6b9b605a5a885357fd54754dc046
---
M be/src/service/hs2-util.cc
1 file changed, 15 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/11976/3
--
To view, visit http://gerrit.cloudera.org:8080/11976
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaec944f2149a6b9b605a5a885357fd54754dc046
Gerrit-Change-Number: 11976
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
[Impala-ASF-CR] IMPALA-7851: fix overflow in ReserveSpace()
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11976 )
Change subject: IMPALA-7851: fix overflow in ReserveSpace()
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11976/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11976/2//COMMIT_MSG@9
PS2, Line 9: The overflow could result in reserving huge amounts of memory in the
Could you provide some more context on why the fix works? eg. how does it not result in us reserving too much memory when previously we had start_idx > 0?
--
To view, visit http://gerrit.cloudera.org:8080/11976
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaec944f2149a6b9b605a5a885357fd54754dc046
Gerrit-Change-Number: 11976
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 21 Nov 2018 21:36:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7851: fix underflow in ReserveSpace()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11976 )
Change subject: IMPALA-7851: fix underflow in ReserveSpace()
......................................................................
Patch Set 2: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/11976
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaec944f2149a6b9b605a5a885357fd54754dc046
Gerrit-Change-Number: 11976
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Nov 2018 23:31:09 +0000
Gerrit-HasComments: No