From 7739d8724b74a899edd3779943243bda5c5aaaa9 Mon Sep 17 00:00:00 2001 From: Kyle Xiao Date: Tue, 30 Jul 2024 11:54:29 +0800 Subject: [PATCH] fix(reflect): zero len slice case (#54) also: * ci: fix not running ./tests --- .github/workflows/pr-check.yml | 30 ++++++++++-------------- .github/workflows/tests.yml | 38 +++++++++++++++++++++++-------- internal/reflect/decoder.go | 34 ++++++++++++++++++++------- internal/reflect/decoder_test.go | 27 +++++++++++++++++++--- internal/reflect/encoder.go | 2 +- internal/reflect/hack.go | 20 ++++++++++++---- internal/reflect/ttype.go | 5 +++- internal/reflect/unknownfields.go | 5 +++- internal/reflect/utils.go | 4 +++- tests/deep_nested_test.go | 13 ----------- tests/frugal_test.go | 9 ++++++++ 11 files changed, 128 insertions(+), 59 deletions(-) diff --git a/.github/workflows/pr-check.yml b/.github/workflows/pr-check.yml index c72216c..c7f8826 100644 --- a/.github/workflows/pr-check.yml +++ b/.github/workflows/pr-check.yml @@ -4,7 +4,7 @@ on: [ pull_request ] jobs: compliant: - runs-on: [ self-hosted, X64 ] + runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 @@ -17,20 +17,13 @@ jobs: uses: crate-ci/typos@master staticcheck: - runs-on: [ self-hosted, X64 ] + runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - name: Set up Go - uses: actions/setup-go@v3 - with: - go-version: "1.22" - - - uses: actions/cache@v3 + uses: actions/setup-go@v5 with: - path: ~/go/pkg/mod - key: reviewdog-${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} - restore-keys: | - reviewdog-${{ runner.os }}-go- + go-version: stable - uses: reviewdog/action-staticcheck@v1 with: @@ -39,23 +32,24 @@ jobs: reporter: github-pr-review # Report all results. filter_mode: nofilter - # Exit with 1 when it find at least one finding. + # Exit with 1 when it finds at least one finding. fail_on_error: true # Set staticcheck flags + # -ST1006 for fixing jit code receiver names like `self` staticcheck_flags: -checks=inherit,-SA1029, -ST1006 lint: - runs-on: [ self-hosted, X64 ] + runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - name: Set up Go - uses: actions/setup-go@v3 + uses: actions/setup-go@v5 with: - go-version: "1.22" + go-version: stable - name: Golangci Lint # https://golangci-lint.run/ - uses: golangci/golangci-lint-action@v3 + uses: golangci/golangci-lint-action@v6 with: version: latest only-new-issues: true diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 0571b93..78f3a3c 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -3,22 +3,42 @@ name: Tests on: [ push, pull_request ] jobs: - unit-benchmark-test: + compatibility-test-amd64: strategy: matrix: go: [ "1.17", "1.18", "1.19", "1.20", "1.21", "1.22" ] os: [ X64 ] runs-on: ${{ matrix.os }} steps: - - uses: actions/checkout@v3 - + - uses: actions/checkout@v4 - name: Set up Go - uses: actions/setup-go@v3 + uses: actions/setup-go@v5 with: go-version: ${{ matrix.go }} + cache: false # don't use cache for self-hosted runners + - name: Test ./... + run: go test -race ./... + - name: Test ./tests + run: cd tests && go test -race + - name: Test Benchmark + run: go test -bench=. -benchmem -run=none ./... -benchtime=100ms - - name: Unit Test - run: go test -race -covermode=atomic -coverprofile=coverage.out ./... - - - name: Benchmark - run: go test -bench=. -benchmem -run=none ./... + compatibility-test-arm64: + strategy: + matrix: + go: [ "1.17", "1.18", "1.19", "1.20", "1.21", "1.22" ] + os: [ ARM64 ] + runs-on: ${{ matrix.os }} + steps: + - uses: actions/checkout@v4 + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: ${{ matrix.go }} + cache: false # don't use cache for self-hosted runners + - name: Test ./... # only tests reflect pkg, JIT pkgs is not supported + run: go test -race ./internal/reflect + - name: Test ./tests + run: cd tests && go test -race + - name: Test Benchmark + run: go test -bench=. -benchmem -run=none ./internal/reflect -benchtime=100ms diff --git a/internal/reflect/decoder.go b/internal/reflect/decoder.go index 7cbda99..213370f 100644 --- a/internal/reflect/decoder.go +++ b/internal/reflect/decoder.go @@ -27,6 +27,18 @@ import ( "github.com/cloudwego/frugal/internal/binary/defs" ) +var ( + zerob = make([]byte, 0) + zerostr = "" + + // for slice, Data should points to zerobase var in `runtime` + // so that it can represent as []type{} instead of []type(nil) + zeroSliceHeader = *(*sliceHeader)(unsafe.Pointer(&zerob)) + + // for string, all fields should be zero + zeroStrHeader = *(*stringHeader)(unsafe.Pointer(&zerostr)) +) + const maxDepthLimit = 1023 var decoderPool = sync.Pool{ @@ -169,17 +181,22 @@ func (d *tDecoder) decodeType(t *tType, b []byte, p unsafe.Pointer, maxdepth int l := int(binary.BigEndian.Uint32(b)) i += 4 if l == 0 { + if t.Tag == defs.T_binary { + *(*sliceHeader)(p) = zeroSliceHeader + } else { + *(*stringHeader)(p) = zeroStrHeader + } return i, nil } x := d.Malloc(l, 1, 0) if t.Tag == defs.T_binary { h := (*sliceHeader)(p) - h.Data = x + h.Data = uintptr(x) h.Len = l h.Cap = l } else { // convert to str h := (*stringHeader)(p) - h.Data = x + h.Data = uintptr(x) h.Len = l } copyn(x, b[i:], l) @@ -193,7 +210,7 @@ func (d *tDecoder) decodeType(t *tType, b []byte, p unsafe.Pointer, maxdepth int // check types kt := t.K vt := t.V - if t0 != kt.T || t1 != vt.T { + if t0 != kt.WT || t1 != vt.WT { return 0, errors.New("type mismatch") } @@ -218,11 +235,11 @@ func (d *tDecoder) decodeType(t *tType, b []byte, p unsafe.Pointer, maxdepth int // instead of // kp = new(type), decode(b, kp) var sliceK unsafe.Pointer - if kt.IsPointer { + if kt.IsPointer && l > 0 { sliceK = d.Malloc(l*kt.V.Size, kt.V.Align, kt.V.MallocAbiType) } var sliceV unsafe.Pointer - if vt.IsPointer { + if vt.IsPointer && l > 0 { sliceV = d.Malloc(l*vt.V.Size, vt.V.Align, vt.V.MallocAbiType) } @@ -271,20 +288,21 @@ func (d *tDecoder) decodeType(t *tType, b []byte, p unsafe.Pointer, maxdepth int // check types et := t.V - if et.T != tp { + if et.WT != tp { return 0, errors.New("type mismatch") } // decode list h := (*sliceHeader)(p) // update the slice field - h.Data = unsafe.Pointer(nil) + h.Data = 0 h.Len = l h.Cap = l if l <= 0 { + *(*sliceHeader)(p) = zeroSliceHeader return i, nil } x := d.Malloc(l*et.Size, et.Align, et.MallocAbiType) // malloc for slice. make([]Type, l, l) - h.Data = x + h.Data = uintptr(x) // pre-allocate space for elements if they're pointers // like diff --git a/internal/reflect/decoder_test.go b/internal/reflect/decoder_test.go index 8ae9dd2..631ebb2 100644 --- a/internal/reflect/decoder_test.go +++ b/internal/reflect/decoder_test.go @@ -89,9 +89,17 @@ func TestDecode(t *testing.T) { test: func(t *testing.T, p1 *TestTypes) { assert.Equal(t, vFloat64, p1.Double) }, }, { - name: "case_string", - update: func(p0 *TestTypes) { p0.String_ = "str" }, - test: func(t *testing.T, p1 *TestTypes) { assert.Equal(t, "str", p1.String_) }, + name: "case_string_binary", + update: func(p0 *TestTypes) { p0.String_ = "str"; p0.Binary = []byte{1} }, + test: func(t *testing.T, p1 *TestTypes) { + assert.Equal(t, "str", p1.String_) + assert.Equal(t, []byte{1}, p1.Binary) + }, + }, + { + name: "case_zero_len_binary", + update: func(p0 *TestTypes) { p0.Binary = []byte{} }, + test: func(t *testing.T, p1 *TestTypes) { assert.Equal(t, []byte{}, p1.Binary) }, }, { name: "case_enum", @@ -136,6 +144,19 @@ func TestDecode(t *testing.T) { assert.Equal(t, []*Msg{{}, {Type: vInt32}}, p1.L2) }, }, + { + name: "case_zero_len_list", + update: func(p0 *TestTypes) { + p0.L0 = []int32{} + p0.L1 = []string{} + p0.L2 = []*Msg{} + }, + test: func(t *testing.T, p1 *TestTypes) { + assert.Equal(t, []int32{}, p1.L0) + assert.Equal(t, []string{}, p1.L1) + assert.Equal(t, []*Msg{}, p1.L2) + }, + }, { name: "case_set", update: func(p0 *TestTypes) { diff --git a/internal/reflect/encoder.go b/internal/reflect/encoder.go index 4471f35..b202fc7 100644 --- a/internal/reflect/encoder.go +++ b/internal/reflect/encoder.go @@ -145,7 +145,7 @@ func (e *tEncoder) encodeContainerType(t *tType, b []byte, p unsafe.Pointer) (in } binary.BigEndian.PutUint32(b[1:], uint32(h.Len)) i := listHeaderLen - vp := h.Data + vp := h.UnsafePointer() // list elements for j := 0; j < h.Len; j++ { if j != 0 { diff --git a/internal/reflect/hack.go b/internal/reflect/hack.go index 1e5d551..cb08617 100644 --- a/internal/reflect/hack.go +++ b/internal/reflect/hack.go @@ -221,19 +221,31 @@ func rtTypePtr(rt reflect.Type) uintptr { return (*iface)(unsafe.Pointer(&rt)).data } -// same as reflect.StringHeader with Data type is unsafe.Pointer +// same as reflect.StringHeader type stringHeader struct { - Data unsafe.Pointer + Data uintptr Len int } -// same as reflect.SliceHeader with Data type is unsafe.Pointer +// UnsafePointer ... for passing checkptr +// `p := unsafe.Pointer(h.Data)` is NOT allowed when testing with -race +func (h *stringHeader) UnsafePointer() unsafe.Pointer { + return *(*unsafe.Pointer)(unsafe.Pointer(h)) +} + +// same as reflect.SliceHeader type sliceHeader struct { - Data unsafe.Pointer + Data uintptr Len int Cap int } +// UnsafePointer ... for passing checkptr +// `p := unsafe.Pointer(h.Data)` is NOT allowed when testing with -race +func (h *sliceHeader) UnsafePointer() unsafe.Pointer { + return *(*unsafe.Pointer)(unsafe.Pointer(h)) +} + //go:linkname mallocgc runtime.mallocgc func mallocgc(size uintptr, typ unsafe.Pointer, needzero bool) unsafe.Pointer diff --git a/internal/reflect/ttype.go b/internal/reflect/ttype.go index a3a26a5..3a9f141 100644 --- a/internal/reflect/ttype.go +++ b/internal/reflect/ttype.go @@ -310,7 +310,10 @@ func (t *tType) encodedListSize(p unsafe.Pointer) (int, error) { return listHeaderLen + (h.Len * vt.FixedSize), nil } ret := listHeaderLen - vp := unsafe.Pointer(h.Data) + if h.Len == 0 { + return ret, nil + } + vp := h.UnsafePointer() for i := 0; i < h.Len; i++ { if i != 0 { vp = unsafe.Add(vp, vt.Size) // move to next element diff --git a/internal/reflect/unknownfields.go b/internal/reflect/unknownfields.go index 341f6c8..737adf0 100644 --- a/internal/reflect/unknownfields.go +++ b/internal/reflect/unknownfields.go @@ -17,6 +17,7 @@ package reflect import ( + "runtime" "sync" "unsafe" ) @@ -47,9 +48,10 @@ func (p *unknownFields) Size() int { func (p *unknownFields) Copy(b []byte) []byte { sz := p.Size() + data := mallocgc(uintptr(sz), nil, false) // without zeroing ret := []byte{} h := (*sliceHeader)(unsafe.Pointer(&ret)) - h.Data = mallocgc(uintptr(sz), nil, false) // without zeroing + h.Data = uintptr(data) h.Len = sz h.Cap = sz off := 0 @@ -57,6 +59,7 @@ func (p *unknownFields) Copy(b []byte) []byte { copy(ret[off:], b[x.off:x.off+x.sz]) off += x.sz } + runtime.KeepAlive(data) return ret } diff --git a/internal/reflect/utils.go b/internal/reflect/utils.go index 655c140..c7a1bbc 100644 --- a/internal/reflect/utils.go +++ b/internal/reflect/utils.go @@ -19,6 +19,7 @@ package reflect import ( "fmt" "reflect" + "runtime" "sync" "unsafe" ) @@ -28,10 +29,11 @@ import ( func copyn(dst unsafe.Pointer, src []byte, n int) { var b []byte hdr := (*sliceHeader)(unsafe.Pointer(&b)) - hdr.Data = dst + hdr.Data = uintptr(dst) hdr.Cap = n hdr.Len = n copy(b, src) + runtime.KeepAlive(dst) } // only be used when NewRequiredFieldNotSetException diff --git a/tests/deep_nested_test.go b/tests/deep_nested_test.go index 9365d5c..f716dc4 100644 --- a/tests/deep_nested_test.go +++ b/tests/deep_nested_test.go @@ -17,7 +17,6 @@ package tests import ( - "runtime" "testing" "github.com/cloudwego/frugal" @@ -95,19 +94,7 @@ var ( ) func TestDeepNested(t *testing.T) { - exit := make(chan int) - go func() { - for { - select { - case <-exit: - t.Logf("exit gc loop") - default: - runtime.GC() - } - } - }() for i := 0; i < 100; i++ { frugal.EncodedSize(deepNested) } - exit <- 0 } diff --git a/tests/frugal_test.go b/tests/frugal_test.go index 379e2db..83bb38b 100644 --- a/tests/frugal_test.go +++ b/tests/frugal_test.go @@ -18,6 +18,7 @@ package tests import ( "reflect" + "runtime" "testing" "github.com/davecgh/go-spew/spew" @@ -30,6 +31,14 @@ import ( "github.com/cloudwego/frugal/tests/kitex_gen/baseline" ) +func init() { + go func() { // it checks unsafe pointer issues + for { + runtime.GC() + } + }() +} + type MyNode struct { Name string `thrift:"Name,1" frugal:"1,default,string" json:"Name"` ID int32 `thrift:"ID,2" frugal:"2,default,i32" json:"ID"`