Skip to content

Commit

Permalink
gopls/internal/lsp/frob: add defensive header
Browse files Browse the repository at this point in the history
I can't explain the crash in the bug report: the frob logic
looks sound, which leaves these possibilities:
(a) the provided data is garbage or is being trampled
    (but the caller logic looks sound);
(b) the file contents are corrupted (but the filecache
    SHA256 checksum was fine);
(c) there's a RAM problem (but that always feels like
    a cop-out explanation).

I've added a magic number to the file header so that there's
a chance we'll detect some variants of a and b.

Updates golang/go#62383

Change-Id: Icd32a2dc6ab019f3deee1b332428e0313c93a6ff
Reviewed-on: https://go-review.googlesource.com/c/tools/+/524655
Run-TryBot: Alan Donovan <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
  • Loading branch information
adonovan committed Aug 31, 2023
1 parent e3671fc commit 8234134
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 7 deletions.
7 changes: 4 additions & 3 deletions gopls/internal/lsp/filecache/filecache.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,11 @@ type memKey struct {
key [32]byte
}

// Get retrieves from the cache and returns a newly allocated
// copy of the value most recently supplied to Set(kind, key),
// possibly by another process.
// Get retrieves from the cache and returns the value most recently
// supplied to Set(kind, key), possibly by another process.
// Get returns ErrNotFound if the value was not found.
//
// Callers should not modify the returned array.
func Get(kind string, key [32]byte) ([]byte, error) {
// First consult the read-through memory cache.
// Note that memory cache hits do not update the times
Expand Down
12 changes: 8 additions & 4 deletions gopls/internal/lsp/frob/frob.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
// Package frob is a fast restricted object encoder/decoder in the
// spirit of encoding/gob.
//
// As with gob, types that recursively contain functions,
// channels, and unsafe.Pointers cannot encoded, but frob has these
// As with gob, types that recursively contain functions, channels,
// and unsafe.Pointers cannot be encoded, but frob has these
// additional restrictions:
//
// - Interface values are not supported; this avoids the need for
Expand All @@ -24,8 +24,6 @@
//
// - There is no error handling. All errors are reported by panicking.
//
// - Types that (recursively) contain private struct fields are not permitted.
//
// - Values are serialized as trees, not graphs, so shared subgraphs
// are encoded repeatedly.
//
Expand Down Expand Up @@ -123,12 +121,15 @@ func (fr *frob) addElem(t reflect.Type) {
fr.elems = append(fr.elems, frobFor(t))
}

const magic = "frob"

func (fr *frob) Encode(v any) []byte {
rv := reflect.ValueOf(v)
if rv.Type() != fr.t {
panic(fmt.Sprintf("got %v, want %v", rv.Type(), fr.t))
}
w := &writer{}
w.bytes([]byte(magic))
fr.encode(w, rv)
if uint64(len(w.data))>>32 != 0 {
panic("too large") // includes all cases where len doesn't fit in 32 bits
Expand Down Expand Up @@ -244,6 +245,9 @@ func (fr *frob) Decode(data []byte, ptr any) {
panic(fmt.Sprintf("got %v, want %v", rv.Type(), fr.t))
}
rd := &reader{data}
if string(rd.bytes(4)) != magic {
panic("not a frob-encoded message")
}
fr.decode(rd, rv)
if len(rd.data) > 0 {
panic("surplus bytes")
Expand Down
4 changes: 4 additions & 0 deletions gopls/internal/lsp/frob/frob_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ func TestBasics(t *testing.T) {
B [2]int
C *Basics
D map[string]int
E []byte
F []string
}
codec := frob.CodecFor[Basics]()

Expand All @@ -29,6 +31,8 @@ func TestBasics(t *testing.T) {
B: [...]int{3, 4},
D: map[string]int{"one": 1},
},
E: []byte("hello"),
F: []string{s1, s2},
}
var y Basics
codec.Decode(codec.Encode(x), &y)
Expand Down

0 comments on commit 8234134

Please sign in to comment.