From 3209fd544b1b8f3486615d29de3c02c8143230b7 Mon Sep 17 00:00:00 2001 From: Xiang Li Date: Tue, 2 Dec 2014 16:19:37 -0800 Subject: [PATCH] raft: panic on bad slice --- raft/log.go | 28 ++++++++----- raft/log_test.go | 98 ++++++++++++++++++++++++++++++++------------ raft/log_unstable.go | 21 ++++------ 3 files changed, 96 insertions(+), 51 deletions(-) diff --git a/raft/log.go b/raft/log.go index 21ac0b9df..6cb1d851d 100644 --- a/raft/log.go +++ b/raft/log.go @@ -217,7 +217,12 @@ func (l *raftLog) term(i uint64) uint64 { panic(err) // TODO(bdarnell) } -func (l *raftLog) entries(i uint64) []pb.Entry { return l.slice(i, l.lastIndex()+1) } +func (l *raftLog) entries(i uint64) []pb.Entry { + if i > l.lastIndex() { + return nil + } + return l.slice(i, l.lastIndex()+1) +} // allEntries returns all entries in the log. func (l *raftLog) allEntries() []pb.Entry { return l.entries(l.firstIndex()) } @@ -250,10 +255,8 @@ func (l *raftLog) restore(s pb.Snapshot) { // slice returns a slice of log entries from lo through hi-1, inclusive. func (l *raftLog) slice(lo uint64, hi uint64) []pb.Entry { - if lo >= hi { - return nil - } - if l.isOutOfBounds(lo) || l.isOutOfBounds(hi-1) { + l.mustCheckOutOfBounds(lo, hi) + if lo == hi { return nil } var ents []pb.Entry @@ -262,9 +265,8 @@ func (l *raftLog) slice(lo uint64, hi uint64) []pb.Entry { if err == ErrCompacted { // This should never fail because it has been checked before. log.Panicf("entries[%d:%d) from storage is out of bound", lo, min(hi, l.unstable.offset)) - return nil } else if err == ErrUnavailable { - return nil + log.Panicf("entries[%d:%d) is unavailable from storage", lo, min(hi, l.unstable.offset)) } else if err != nil { panic(err) // TODO(bdarnell) } @@ -277,9 +279,13 @@ func (l *raftLog) slice(lo uint64, hi uint64) []pb.Entry { return ents } -func (l *raftLog) isOutOfBounds(i uint64) bool { - if i < l.firstIndex() || i > l.lastIndex() { - return true +// l.firstIndex <= lo <= hi <= l.firstIndex + len(l.entries) +func (l *raftLog) mustCheckOutOfBounds(lo, hi uint64) { + if lo > hi { + log.Panicf("raft: invalid slice %d > %d", lo, hi) + } + length := l.lastIndex() - l.firstIndex() + 1 + if lo < l.firstIndex() || hi > l.firstIndex()+length { + log.Panicf("raft: slice[%d,%d) out of bound [%d,%d]", lo, hi, l.firstIndex(), l.lastIndex()) } - return false } diff --git a/raft/log_test.go b/raft/log_test.go index 9a5829b59..609654c60 100644 --- a/raft/log_test.go +++ b/raft/log_test.go @@ -258,7 +258,7 @@ func TestLogMaybeAppend(t *testing.T) { if gcommit != tt.wcommit { t.Errorf("#%d: committed = %d, want %d", i, gcommit, tt.wcommit) } - if gappend { + if gappend && len(tt.ents) != 0 { gents := raftLog.slice(raftLog.lastIndex()-uint64(len(tt.ents))+1, raftLog.lastIndex()+1) if !reflect.DeepEqual(tt.ents, gents) { t.Errorf("%d: appended entries = %v, want %v", i, gents, tt.ents) @@ -572,22 +572,59 @@ func TestIsOutOfBounds(t *testing.T) { l.append(pb.Entry{Index: i + offset}) } + first := offset + 1 tests := []struct { - index uint64 - w bool + lo, hi uint64 + wpainc bool }{ - {offset - 1, true}, - {offset, true}, - {offset + num/2, false}, - {offset + num, false}, - {offset + num + 1, true}, + { + first - 2, first + 1, + true, + }, + { + first - 1, first + 1, + true, + }, + { + first, first, + false, + }, + { + first + num/2, first + num/2, + false, + }, + { + first + num - 1, first + num - 1, + false, + }, + { + first + num, first + num, + false, + }, + { + first + num, first + num + 1, + true, + }, + { + first + num + 1, first + num + 1, + true, + }, } for i, tt := range tests { - g := l.isOutOfBounds(tt.index) - if g != tt.w { - t.Errorf("#%d: isOutOfBounds = %v, want %v", i, g, tt.w) - } + func() { + defer func() { + if r := recover(); r != nil { + if !tt.wpainc { + t.Errorf("%d: panic = %v, want %v: %v", i, true, false, r) + } + } + }() + l.mustCheckOutOfBounds(tt.lo, tt.hi) + if tt.wpainc { + t.Errorf("%d: panic = %v, want %v", i, false, true) + } + }() } } @@ -635,24 +672,31 @@ func TestSlice(t *testing.T) { } tests := []struct { - from uint64 - to uint64 - w []pb.Entry + from uint64 + to uint64 + w []pb.Entry + wpanic bool }{ - {offset - 1, offset + 1, nil}, - {offset, offset + 1, nil}, - {offset + num/2, offset + num/2 + 1, []pb.Entry{{Index: offset + num/2, Term: offset + num/2}}}, - {offset + num - 1, offset + num, []pb.Entry{{Index: offset + num - 1, Term: offset + num - 1}}}, - {offset + num, offset + num + 1, nil}, - - {offset + num/2, offset + num/2, nil}, - {offset + num/2, offset + num/2 - 1, nil}, + {offset - 1, offset + 1, nil, true}, + {offset, offset + 1, nil, true}, + {offset + num/2, offset + num/2 + 1, []pb.Entry{{Index: offset + num/2, Term: offset + num/2}}, false}, + {offset + num - 1, offset + num, []pb.Entry{{Index: offset + num - 1, Term: offset + num - 1}}, false}, + {offset + num, offset + num + 1, nil, true}, } for i, tt := range tests { - g := l.slice(tt.from, tt.to) - if !reflect.DeepEqual(g, tt.w) { - t.Errorf("#%d: from %d to %d = %v, want %v", i, tt.from, tt.to, g, tt.w) - } + func() { + defer func() { + if r := recover(); r != nil { + if !tt.wpanic { + t.Errorf("%d: panic = %v, want %v: %v", i, true, false, r) + } + } + }() + g := l.slice(tt.from, tt.to) + if !reflect.DeepEqual(g, tt.w) { + t.Errorf("#%d: from %d to %d = %v, want %v", i, tt.from, tt.to, g, tt.w) + } + }() } } diff --git a/raft/log_unstable.go b/raft/log_unstable.go index fa7d3b8d0..93ead6be4 100644 --- a/raft/log_unstable.go +++ b/raft/log_unstable.go @@ -127,22 +127,17 @@ func (u *unstable) truncateAndAppend(ents []pb.Entry) { } func (u *unstable) slice(lo uint64, hi uint64) []pb.Entry { - if lo >= hi { - return nil - } - if u.isOutOfBounds(lo) || u.isOutOfBounds(hi-1) { - return nil - } + u.mustCheckOutOfBounds(lo, hi) return u.entries[lo-u.offset : hi-u.offset] } -func (u *unstable) isOutOfBounds(i uint64) bool { - if len(u.entries) == 0 { - return true +// u.offset <= lo <= hi <= u.offset+len(u.offset) +func (u *unstable) mustCheckOutOfBounds(lo, hi uint64) { + if lo > hi { + log.Panicf("raft: invalid unstable.slice %d > %d", lo, hi) } - last := u.offset + uint64(len(u.entries)) - 1 - if i < u.offset || i > last { - return true + upper := u.offset + uint64(len(u.entries)) + if lo < u.offset || hi > upper { + log.Panicf("raft: unstable.slice[%d,%d) out of bound [%d,%d]", lo, hi, u.offset, upper) } - return false }