You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Joe McDonnell (Code Review)" <ge...@cloudera.org> on 2023/05/10 22:34:37 UTC
[Impala-ASF-CR] IMPALA-12123: Fix crash triggered by incomplete HDFS cache reads
Joe McDonnell has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19869
Change subject: IMPALA-12123: Fix crash triggered by incomplete HDFS cache reads
......................................................................
IMPALA-12123: Fix crash triggered by incomplete HDFS cache reads
When using HDFS caching, the HDFS cache may not have the full
buffer in memory, and it can return a buffer that is incomplete.
In this case, the code falls back to the ordinary read path.
However, the ScanRange cache_ structure is still set up, and
the code in ScanRange::ReadSubRanges() tries to use it. This
can crash, because the buffer is too short (and may have been
freed).
This changes the code to null out the cache_ data structure
when there is an incomplete read from the HDFS cache.
Testing:
- Reproduced the crash stack manually by putting a Parquet
file with a page index in HDFS cache and manually forcing
it down the incomplete read codepath.
- Modified the disk-io-mgr-test and CacheReaderTestStub to
simulate the incomplete read case. The test will hit a
DCHECK or crash without this fixup.
Change-Id: I51d8be6c03716badee81675447ed94ae6249b21b
---
M be/src/runtime/io/cache-reader-test-stub.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/scan-range.cc
3 files changed, 87 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/19869/1
--
To view, visit http://gerrit.cloudera.org:8080/19869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I51d8be6c03716badee81675447ed94ae6249b21b
Gerrit-Change-Number: 19869
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
[Impala-ASF-CR] IMPALA-12123: Fix crash triggered by incomplete HDFS cache reads
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/19869 )
Change subject: IMPALA-12123: Fix crash triggered by incomplete HDFS cache reads
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19869/1/be/src/runtime/io/scan-range.cc
File be/src/runtime/io/scan-range.cc:
http://gerrit.cloudera.org:8080/#/c/19869/1/be/src/runtime/io/scan-range.cc@344
PS1, Line 344: if (cache_.data != nullptr) {
: DCHECK_LE(offset + bytes_to_read, cache_.len);
: memcpy(buffer_desc->buffer_ + buffer_desc->len(),
: cache_.data + offset, bytes_to_read);
: } else {
My assumption here is that we already did checks up in the Parquet level so even if it was reading a corrupted / invalid file, we won't have subranges that go past the edge of this buffer. The other option would be to return Status rather than DCHECKing.
It is complicated to exercise this line in the end-to-end minicluster with real HDFS caching, so the DCHECK isn't likely to get exercised or provide much protection.
--
To view, visit http://gerrit.cloudera.org:8080/19869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51d8be6c03716badee81675447ed94ae6249b21b
Gerrit-Change-Number: 19869
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 May 2023 23:20:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12123: Fix crash triggered by incomplete HDFS cache reads
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19869 )
Change subject: IMPALA-12123: Fix crash triggered by incomplete HDFS cache reads
......................................................................
Patch Set 1:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9304/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/19869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51d8be6c03716badee81675447ed94ae6249b21b
Gerrit-Change-Number: 19869
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 May 2023 22:43:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12123: Fix crash triggered by incomplete HDFS cache reads
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19869 )
Change subject: IMPALA-12123: Fix crash triggered by incomplete HDFS cache reads
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/19869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51d8be6c03716badee81675447ed94ae6249b21b
Gerrit-Change-Number: 19869
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 May 2023 23:02:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12123: Fix crash triggered by incomplete HDFS cache reads
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/19869 )
Change subject: IMPALA-12123: Fix crash triggered by incomplete HDFS cache reads
......................................................................
Patch Set 1: Code-Review+2
Thanks for fixing this!
--
To view, visit http://gerrit.cloudera.org:8080/19869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51d8be6c03716badee81675447ed94ae6249b21b
Gerrit-Change-Number: 19869
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 11 May 2023 12:00:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12123: Fix crash triggered by incomplete HDFS cache reads
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19869 )
Change subject: IMPALA-12123: Fix crash triggered by incomplete HDFS cache reads
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12990/ : 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/19869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51d8be6c03716badee81675447ed94ae6249b21b
Gerrit-Change-Number: 19869
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 May 2023 22:56:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12123: Fix crash triggered by incomplete HDFS cache reads
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19869 )
Change subject: IMPALA-12123: Fix crash triggered by incomplete HDFS cache reads
......................................................................
IMPALA-12123: Fix crash triggered by incomplete HDFS cache reads
When using HDFS caching, the HDFS cache may not have the full
buffer in memory, and it can return a buffer that is incomplete.
In this case, the code falls back to the ordinary read path.
However, the ScanRange cache_ structure is still set up, and
the code in ScanRange::ReadSubRanges() tries to use it. This
can crash, because the buffer is too short (and may have been
freed).
This changes the code to null out the cache_ data structure
when there is an incomplete read from the HDFS cache.
Testing:
- Reproduced the crash stack manually by putting a Parquet
file with a page index in HDFS cache and manually forcing
it down the incomplete read codepath.
- Modified the disk-io-mgr-test and CacheReaderTestStub to
simulate the incomplete read case. The test will hit a
DCHECK or crash without this fixup.
Change-Id: I51d8be6c03716badee81675447ed94ae6249b21b
Reviewed-on: http://gerrit.cloudera.org:8080/19869
Reviewed-by: Michael Smith <mi...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Zoltan Borok-Nagy <bo...@cloudera.com>
---
M be/src/runtime/io/cache-reader-test-stub.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/scan-range.cc
3 files changed, 87 insertions(+), 47 deletions(-)
Approvals:
Michael Smith: Looks good to me, but someone else must approve
Impala Public Jenkins: Verified
Zoltan Borok-Nagy: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/19869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I51d8be6c03716badee81675447ed94ae6249b21b
Gerrit-Change-Number: 19869
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-12123: Fix crash triggered by incomplete HDFS cache reads
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19869 )
Change subject: IMPALA-12123: Fix crash triggered by incomplete HDFS cache reads
......................................................................
Patch Set 1: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/19869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51d8be6c03716badee81675447ed94ae6249b21b
Gerrit-Change-Number: 19869
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 11 May 2023 04:03:07 +0000
Gerrit-HasComments: No