You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/10/05 23:38:48 UTC
[kudu-CR] log: address a heap overflow race during log roll
Hello Dan Burkert, Mike Percy,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4638
to review the following change.
Change subject: log: address a heap overflow race during log roll
......................................................................
log: address a heap overflow race during log roll
This addresses the following heap overflow which I managed to produce in
TSAN builds of raft-consensus-itest:
==14633==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000083b58 at pc 0x7f95761db463 bp 0x7f9532d1ad00 sp 0x7f9532d1acf8
READ of size 8 at 0x602000083b58 thread T115 (maintenance_sch)
#0 0x7f95761db462 in scoped_refptr<kudu::log::ReadableLogSegment>::operator->() const /home/todd/git/kudu/build/asan/../../src/kudu/gutil/ref_counted.h:273:5
#1 0x7f957577a012 in kudu::log::Log::GetMaxIndexesToSegmentSizeMap(long, std::map<long, long, std::less<long>, std::allocator<std::pair<long const, long> > >*) const /home/todd/git/kudu/build/asan/../../src/kudu/consensus/log.cc:787:10
#2 0x7f9575c5995a in kudu::tablet::TabletPeer::GetMaxIndexesToSegmentSizeMap(std::map<long, long, std::less<long>, std::allocator<std::pair<long const, long> > >*) const /home/todd/git/kudu/build/asan/../../src/kudu/tablet/tablet_peer.cc:480:9
The issue is the following race:
- we grab a snapshot of the current log segments
- the log rolls, such that the last segment is now closed and has a
footer
- FLAGS_log_min_segments_to_retain is higher than segments.size()
In this case, we triggered integer underflow in the following for loop:
for (int i = gc_prefix; i < segments.size() - FLAGS_log_min_segments_to_retain; i++) {
since segments.size() is an unsigned integer. This made the loop never
terminate. In most cases, it would terminate due to the last segment
having no footer, but in the race described above, that would not be the
case. We'd then access invalid memory and probably crash.
The test modifications in this patch would trigger the issue reliably
when run with --stress_cpu_threads=5.
Change-Id: Ic6dd40cf9ac628f56f0c467bcd9eeb2191124836
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/mt-log-test.cc
2 files changed, 28 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/4638/1
--
To view, visit http://gerrit.cloudera.org:8080/4638
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic6dd40cf9ac628f56f0c467bcd9eeb2191124836
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] log: add a test for race conditions
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4638
to look at the new patch set (#2).
Change subject: log: add a test for race conditions
......................................................................
log: add a test for race conditions
This adds new test coverage in mt-log-test to look for races between
log rolls and other operations. It doesn't currently find any bug, but
prior to 127438af30356f1afedb862166c907ff754d1c55 it reproduced a crash
bug fairly reliably, so the additional test coverage is probably worth
having.
Change-Id: Ic6dd40cf9ac628f56f0c467bcd9eeb2191124836
---
M src/kudu/consensus/mt-log-test.cc
1 file changed, 24 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/4638/2
--
To view, visit http://gerrit.cloudera.org:8080/4638
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic6dd40cf9ac628f56f0c467bcd9eeb2191124836
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] log: add a test for race conditions
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: log: add a test for race conditions
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4638/2/src/kudu/consensus/mt-log-test.cc
File src/kudu/consensus/mt-log-test.cc:
Line 176: FLAGS_num_writer_threads * FLAGS_num_batches_per_thread,
unrelated, but this should be FLAGS_num_writer_threads only
--
To view, visit http://gerrit.cloudera.org:8080/4638
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6dd40cf9ac628f56f0c467bcd9eeb2191124836
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] log: add a test for race conditions
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: log: add a test for race conditions
......................................................................
Patch Set 2:
well, turns out that this bug no longer exists after the relevant code was rewritten by 127438af30356f1afedb862166c907ff754d1c55. So now this patch is just the new test coverage.
--
To view, visit http://gerrit.cloudera.org:8080/4638
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6dd40cf9ac628f56f0c467bcd9eeb2191124836
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] log: address a heap overflow race during log roll
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: log: address a heap overflow race during log roll
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4638
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6dd40cf9ac628f56f0c467bcd9eeb2191124836
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: No
[kudu-CR] log: add a test for race conditions
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: log: add a test for race conditions
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4638/2/src/kudu/consensus/mt-log-test.cc
File src/kudu/consensus/mt-log-test.cc:
Line 176: FLAGS_num_writer_threads * FLAGS_num_batches_per_thread,
> unrelated, but this should be FLAGS_num_writer_threads only
not sure I follow. though I think the two flags on the following line are in backwards order.
--
To view, visit http://gerrit.cloudera.org:8080/4638
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6dd40cf9ac628f56f0c467bcd9eeb2191124836
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] log: add a test for race conditions
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: log: add a test for race conditions
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4638/2/src/kudu/consensus/mt-log-test.cc
File src/kudu/consensus/mt-log-test.cc:
Line 176: FLAGS_num_writer_threads * FLAGS_num_batches_per_thread,
> not sure I follow. though I think the two flags on the following line are i
oh. yeah. that's true. anyway it's not very important, it's a test
--
To view, visit http://gerrit.cloudera.org:8080/4638
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6dd40cf9ac628f56f0c467bcd9eeb2191124836
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] log: add a test for race conditions
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.
Change subject: log: add a test for race conditions
......................................................................
log: add a test for race conditions
This adds new test coverage in mt-log-test to look for races between
log rolls and other operations. It doesn't currently find any bug, but
prior to 127438af30356f1afedb862166c907ff754d1c55 it reproduced a crash
bug fairly reliably, so the additional test coverage is probably worth
having.
Change-Id: Ic6dd40cf9ac628f56f0c467bcd9eeb2191124836
Reviewed-on: http://gerrit.cloudera.org:8080/4638
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/consensus/mt-log-test.cc
1 file changed, 24 insertions(+), 0 deletions(-)
Approvals:
Mike Percy: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4638
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic6dd40cf9ac628f56f0c467bcd9eeb2191124836
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>