From db87efa2ec1e91e81517236b164b279e57b8daa8 Mon Sep 17 00:00:00 2001 From: Ondrej Fabry Date: Fri, 20 Mar 2020 10:52:19 +0100 Subject: Fix statsclient for VPP 20.05-rc0 (master) - this change fixes panic that was occurring with recent VPP that was caused by incorrectly calculated vector length - converting returned vector length from uint64 to uint32 results in correct length value Change-Id: I76a4b9d147c3df3bea9d3e5ef5853e2809dc42e8 Signed-off-by: Ondrej Fabry --- CHANGELOG.md | 6 +++ adapter/stats_api.go | 4 +- adapter/statsclient/stat_segment.go | 86 +++++++++++++++++++++---------------- adapter/statsclient/statsclient.go | 61 +++++++++++++------------- adapter/statsclient/statseg.go | 18 +++++--- version/version.go | 2 +- 6 files changed, 102 insertions(+), 75 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 088c84d..72c7a27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,12 @@ This file lists changes for the GoVPP releases. - --> +## 0.3.2 +> _20 March 2020_ + +### Fixes +- statsclient: Fix panic occurring with VPP 20.05-rc0 (master) + ## 0.3.0 > _18 March 2020_ diff --git a/adapter/stats_api.go b/adapter/stats_api.go index d67434c..90dbeb3 100644 --- a/adapter/stats_api.go +++ b/adapter/stats_api.go @@ -113,7 +113,7 @@ func (n Name) String() string { return string(n) } -// Data represents some type of stat which is usually defined by StatType. +// Stat represents some type of stat which is usually defined by StatType. type Stat interface { // IsZero returns true if all of its values equal to zero. IsZero() bool @@ -205,7 +205,7 @@ func ReduceSimpleCounterStatIndex(s SimpleCounterStat, i int) uint64 { return val } -// ReduceSimpleCounterStatIndex returns reduced CombinedCounterStat s for index i. +// ReduceCombinedCounterStatIndex returns reduced CombinedCounterStat s for index i. func ReduceCombinedCounterStatIndex(s CombinedCounterStat, i int) [2]uint64 { var val [2]uint64 for _, w := range s { diff --git a/adapter/statsclient/stat_segment.go b/adapter/statsclient/stat_segment.go index 9f028eb..0d988ba 100644 --- a/adapter/statsclient/stat_segment.go +++ b/adapter/statsclient/stat_segment.go @@ -52,20 +52,11 @@ type statSegment struct { legacyVersion bool } -func (c *statSegment) getStatDirVector() unsafe.Pointer { - dirOffset, _, _ := c.getOffsets() - return unsafe.Pointer(&c.sharedHeader[dirOffset]) -} - -func (c *statSegment) getStatDirIndex(p unsafe.Pointer, index uint32) *statSegDirectoryEntry { - return (*statSegDirectoryEntry)(unsafe.Pointer(uintptr(p) + uintptr(index)*unsafe.Sizeof(statSegDirectoryEntry{}))) -} - -func (c *statSegment) getHeader() (header statSegSharedHeader) { +func (c *statSegment) getHeader() (header sharedHeader) { if c.legacyVersion { - return statSegHeaderLegacy(c.sharedHeader) + return loadSharedHeaderLegacy(c.sharedHeader) } - return statSegHeader(c.sharedHeader) + return loadSharedHeader(c.sharedHeader) } func (c *statSegment) getEpoch() (int64, bool) { @@ -129,17 +120,17 @@ func (c *statSegment) connect(sockName string) error { return fmt.Errorf("mapping shared memory failed: %v", err) } + Log.Debugf("successfuly mmapped shared memory segment (size: %v) %v", size, len(data)) + c.sharedHeader = data c.memorySize = size - Log.Debugf("successfuly mmapped shared memory segment (size: %v)", size) - - hdr := statSegHeader(c.sharedHeader) + hdr := loadSharedHeader(c.sharedHeader) Log.Debugf("stat segment header: %+v", hdr) if hdr.legacyVersion() { c.legacyVersion = true - hdr = statSegHeaderLegacy(c.sharedHeader) + hdr = loadSharedHeaderLegacy(c.sharedHeader) Log.Debugf("falling back to legacy version (VPP <=19.04) of stat segment (header: %+v)", hdr) } @@ -167,6 +158,16 @@ func (c *statSegment) disconnect() error { type statDirectoryType int32 +const ( + statDirIllegal = 0 + statDirScalarIndex = 1 + statDirCounterVectorSimple = 2 + statDirCounterVectorCombined = 3 + statDirErrorIndex = 4 + statDirNameVector = 5 + statDirEmpty = 6 +) + func (t statDirectoryType) String() string { return adapter.StatType(t).String() } @@ -182,14 +183,23 @@ type statSegDirectoryEntry struct { name [128]byte } +func (c *statSegment) getStatDirVector() unsafe.Pointer { + dirOffset, _, _ := c.getOffsets() + return unsafe.Pointer(&c.sharedHeader[dirOffset]) +} + +func (c *statSegment) getStatDirIndex(p unsafe.Pointer, index uint32) *statSegDirectoryEntry { + return (*statSegDirectoryEntry)(unsafe.Pointer(uintptr(p) + uintptr(index)*unsafe.Sizeof(statSegDirectoryEntry{}))) +} + func (c *statSegment) copyEntryData(dirEntry *statSegDirectoryEntry) adapter.Stat { dirType := adapter.StatType(dirEntry.directoryType) switch dirType { - case adapter.ScalarIndex: + case statDirScalarIndex: return adapter.ScalarStat(dirEntry.unionData) - case adapter.ErrorIndex: + case statDirErrorIndex: _, errOffset, _ := c.getOffsets() offsetVector := unsafe.Pointer(&c.sharedHeader[errOffset]) @@ -200,17 +210,19 @@ func (c *statSegment) copyEntryData(dirEntry *statSegDirectoryEntry) adapter.Sta val := *(*adapter.Counter)(statSegPointer(offsetVector, offset)) errData = val } else { - vecLen := vectorLen(offsetVector) - for i := uint64(0); i < vecLen; i++ { + vecLen := uint32(vectorLen(offsetVector)) + + for i := uint32(0); i < vecLen; i++ { cb := *(*uint64)(statSegPointer(offsetVector, uintptr(i)*unsafe.Sizeof(uint64(0)))) offset := uintptr(cb) + uintptr(dirEntry.unionData)*unsafe.Sizeof(adapter.Counter(0)) + debugf("error index, cb: %d, offset: %d", cb, offset) val := *(*adapter.Counter)(statSegPointer(unsafe.Pointer(&c.sharedHeader[0]), offset)) errData += val } } return adapter.ErrorStat(errData) - case adapter.SimpleCounterVector: + case statDirCounterVectorSimple: if dirEntry.unionData == 0 { debugf("offset invalid for %s", dirEntry.name) break @@ -219,16 +231,16 @@ func (c *statSegment) copyEntryData(dirEntry *statSegDirectoryEntry) adapter.Sta break } - vecLen := vectorLen(unsafe.Pointer(&c.sharedHeader[dirEntry.unionData])) + vecLen := uint32(vectorLen(unsafe.Pointer(&c.sharedHeader[dirEntry.unionData]))) offsetVector := statSegPointer(unsafe.Pointer(&c.sharedHeader[0]), uintptr(dirEntry.offsetVector)) data := make([][]adapter.Counter, vecLen) - for i := uint64(0); i < vecLen; i++ { + for i := uint32(0); i < vecLen; i++ { cb := *(*uint64)(statSegPointer(offsetVector, uintptr(i)*unsafe.Sizeof(uint64(0)))) counterVec := unsafe.Pointer(&c.sharedHeader[uintptr(cb)]) - vecLen2 := vectorLen(counterVec) + vecLen2 := uint32(vectorLen(counterVec)) data[i] = make([]adapter.Counter, vecLen2) - for j := uint64(0); j < vecLen2; j++ { + for j := uint32(0); j < vecLen2; j++ { offset := uintptr(j) * unsafe.Sizeof(adapter.Counter(0)) val := *(*adapter.Counter)(statSegPointer(counterVec, offset)) data[i][j] = val @@ -236,7 +248,7 @@ func (c *statSegment) copyEntryData(dirEntry *statSegDirectoryEntry) adapter.Sta } return adapter.SimpleCounterStat(data) - case adapter.CombinedCounterVector: + case statDirCounterVectorCombined: if dirEntry.unionData == 0 { debugf("offset invalid for %s", dirEntry.name) break @@ -245,16 +257,16 @@ func (c *statSegment) copyEntryData(dirEntry *statSegDirectoryEntry) adapter.Sta break } - vecLen := vectorLen(unsafe.Pointer(&c.sharedHeader[dirEntry.unionData])) + vecLen := uint32(vectorLen(unsafe.Pointer(&c.sharedHeader[dirEntry.unionData]))) offsetVector := statSegPointer(unsafe.Pointer(&c.sharedHeader[0]), uintptr(dirEntry.offsetVector)) data := make([][]adapter.CombinedCounter, vecLen) - for i := uint64(0); i < vecLen; i++ { + for i := uint32(0); i < vecLen; i++ { cb := *(*uint64)(statSegPointer(offsetVector, uintptr(i)*unsafe.Sizeof(uint64(0)))) counterVec := unsafe.Pointer(&c.sharedHeader[uintptr(cb)]) - vecLen2 := vectorLen(counterVec) + vecLen2 := uint32(vectorLen(counterVec)) data[i] = make([]adapter.CombinedCounter, vecLen2) - for j := uint64(0); j < vecLen2; j++ { + for j := uint32(0); j < vecLen2; j++ { offset := uintptr(j) * unsafe.Sizeof(adapter.CombinedCounter{}) val := *(*adapter.CombinedCounter)(statSegPointer(counterVec, offset)) data[i][j] = val @@ -262,7 +274,7 @@ func (c *statSegment) copyEntryData(dirEntry *statSegDirectoryEntry) adapter.Sta } return adapter.CombinedCounterStat(data) - case adapter.NameVector: + case statDirNameVector: if dirEntry.unionData == 0 { debugf("offset invalid for %s", dirEntry.name) break @@ -271,21 +283,21 @@ func (c *statSegment) copyEntryData(dirEntry *statSegDirectoryEntry) adapter.Sta break } - vecLen := vectorLen(unsafe.Pointer(&c.sharedHeader[dirEntry.unionData])) + vecLen := uint32(vectorLen(unsafe.Pointer(&c.sharedHeader[dirEntry.unionData]))) offsetVector := statSegPointer(unsafe.Pointer(&c.sharedHeader[0]), uintptr(dirEntry.offsetVector)) data := make([]adapter.Name, vecLen) - for i := uint64(0); i < vecLen; i++ { + for i := uint32(0); i < vecLen; i++ { cb := *(*uint64)(statSegPointer(offsetVector, uintptr(i)*unsafe.Sizeof(uint64(0)))) if cb == 0 { debugf("name vector out of range for %s (%v)", dirEntry.name, i) continue } nameVec := unsafe.Pointer(&c.sharedHeader[cb]) - vecLen2 := vectorLen(nameVec) + vecLen2 := uint32(vectorLen(nameVec)) nameStr := make([]byte, 0, vecLen2) - for j := uint64(0); j < vecLen2; j++ { + for j := uint32(0); j < vecLen2; j++ { offset := uintptr(j) * unsafe.Sizeof(byte(0)) val := *(*byte)(statSegPointer(nameVec, offset)) if val > 0 { @@ -296,11 +308,13 @@ func (c *statSegment) copyEntryData(dirEntry *statSegDirectoryEntry) adapter.Sta } return adapter.NameStat(data) + case statDirEmpty: + // no-op + default: // TODO: monitor occurrences with metrics debugf("Unknown type %d for stat entry: %q", dirEntry.directoryType, dirEntry.name) } - return nil } diff --git a/adapter/statsclient/statsclient.go b/adapter/statsclient/statsclient.go index d4e5a56..9110275 100644 --- a/adapter/statsclient/statsclient.go +++ b/adapter/statsclient/statsclient.go @@ -121,12 +121,24 @@ func (c *StatsClient) ListStats(patterns ...string) (names []string, err error) if err != nil { return nil, err } + + dirVector := c.getStatDirVector() + vecLen := uint32(vectorLen(dirVector)) + for _, index := range indexes { - name, err := c.entryName(index) - if err != nil { - return nil, err + if index >= vecLen { + return nil, fmt.Errorf("stat entry index %d out of dir vector len (%d)", index, vecLen) } - names = append(names, name) + + dirEntry := c.getStatDirIndex(dirVector, index) + var name []byte + for n := 0; n < len(dirEntry.name); n++ { + if dirEntry.name[n] == 0 { + name = dirEntry.name[:n] + break + } + } + names = append(names, string(name)) } if !c.accessEnd(&sa) { @@ -142,11 +154,11 @@ func (c *StatsClient) DumpStats(patterns ...string) (entries []adapter.StatEntry return nil, adapter.ErrStatsAccessFailed } - dir, err := c.listIndexes(patterns...) + indexes, err := c.listIndexes(patterns...) if err != nil { return nil, err } - if entries, err = c.dumpEntries(dir); err != nil { + if entries, err = c.dumpEntries(indexes); err != nil { return nil, err } @@ -261,7 +273,7 @@ func (c *StatsClient) listIndexes(patterns ...string) (indexes []uint32, err err func (c *StatsClient) listIndexesFunc(f func(name []byte) bool) (indexes []uint32, err error) { if f == nil { - // there is around ~3150 stats, so to avoid too many allocations + // there is around ~3157 stats, so to avoid too many allocations // we set capacity to 3200 when listing all stats indexes = make([]uint32, 0, 3200) } @@ -290,33 +302,18 @@ func (c *StatsClient) listIndexesFunc(f func(name []byte) bool) (indexes []uint3 return indexes, nil } -func (c *StatsClient) entryName(index uint32) (string, error) { +func (c *StatsClient) dumpEntries(indexes []uint32) (entries []adapter.StatEntry, err error) { dirVector := c.getStatDirVector() - vecLen := uint32(vectorLen(dirVector)) + dirLen := uint32(vectorLen(dirVector)) - if index >= vecLen { - return "", fmt.Errorf("stat entry index %d out of range (%d)", index, vecLen) - } - - dirEntry := c.getStatDirIndex(dirVector, index) - - var name []byte - for n := 0; n < len(dirEntry.name); n++ { - if dirEntry.name[n] == 0 { - name = dirEntry.name[:n] - break - } - } - - return string(name), nil -} + debugf("dumping entres for %d indexes", len(indexes)) -func (c *StatsClient) dumpEntries(indexes []uint32) (entries []adapter.StatEntry, err error) { entries = make([]adapter.StatEntry, 0, len(indexes)) - - dirVector := c.getStatDirVector() - for _, index := range indexes { + if index >= dirLen { + return nil, fmt.Errorf("stat entry index %d out of dir vector length (%d)", index, dirLen) + } + dirEntry := c.getStatDirIndex(dirVector, index) var name []byte @@ -326,6 +323,12 @@ func (c *StatsClient) dumpEntries(indexes []uint32) (entries []adapter.StatEntry break } } + + if Debug { + debugf(" - %3d. dir: %q type: %v offset: %d union: %d", index, name, + adapter.StatType(dirEntry.directoryType), dirEntry.offsetVector, dirEntry.unionData) + } + if len(name) == 0 { continue } diff --git a/adapter/statsclient/statseg.go b/adapter/statsclient/statseg.go index 7f1c381..42ab3de 100644 --- a/adapter/statsclient/statseg.go +++ b/adapter/statsclient/statseg.go @@ -19,12 +19,16 @@ type sharedHeaderBase struct { statsOffset int64 } -type statSegSharedHeader struct { +type sharedHeaderV0 struct { + sharedHeaderBase +} + +type sharedHeader struct { version uint64 sharedHeaderBase } -func (h *statSegSharedHeader) legacyVersion() bool { +func (h *sharedHeader) legacyVersion() bool { // older VPP (<=19.04) did not have version in stat segment header // we try to provide fallback support by skipping it in header if h.version > maxVersion && h.inProgress > 1 && h.epoch == 0 { @@ -33,8 +37,8 @@ func (h *statSegSharedHeader) legacyVersion() bool { return false } -func statSegHeader(b []byte) (header statSegSharedHeader) { - h := (*statSegSharedHeader)(unsafe.Pointer(&b[0])) +func loadSharedHeader(b []byte) (header sharedHeader) { + h := (*sharedHeader)(unsafe.Pointer(&b[0])) header.version = atomic.LoadUint64(&h.version) header.epoch = atomic.LoadInt64(&h.epoch) header.inProgress = atomic.LoadInt64(&h.inProgress) @@ -44,8 +48,8 @@ func statSegHeader(b []byte) (header statSegSharedHeader) { return } -func statSegHeaderLegacy(b []byte) (header statSegSharedHeader) { - h := (*sharedHeaderBase)(unsafe.Pointer(&b[0])) +func loadSharedHeaderLegacy(b []byte) (header sharedHeader) { + h := (*sharedHeaderV0)(unsafe.Pointer(&b[0])) header.version = 0 header.epoch = atomic.LoadInt64(&h.epoch) header.inProgress = atomic.LoadInt64(&h.inProgress) @@ -90,7 +94,7 @@ type vecHeader struct { } func vectorLen(v unsafe.Pointer) uint64 { - vec := *(*vecHeader)(unsafe.Pointer(uintptr(v) - unsafe.Sizeof(uintptr(0)))) + vec := *(*vecHeader)(unsafe.Pointer(uintptr(v) - unsafe.Sizeof(uint64(0)))) return vec.length } diff --git a/version/version.go b/version/version.go index 36eaa3a..ed5922e 100644 --- a/version/version.go +++ b/version/version.go @@ -23,7 +23,7 @@ import ( var ( name = "govpp" - version = "v0.3.0-dev" + version = "v0.3.2" commitHash = "unknown" buildBranch = "HEAD" buildStamp = "" -- cgit 1.2.3-korg