Merge pull request #1836 from xiang90/panic_slice

raft: panic on bad slice

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
 }

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)
+			}
+		}()
 	}
 }

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
 }