Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

memdb: compatible with Go 1.14's checkptr #14972

Merged
merged 4 commits into from
Feb 28, 2020

Conversation

bobotu
Copy link
Contributor

@bobotu bobotu commented Feb 27, 2020

What problem does this PR solve?

The Go 1.14 introduces new instrumentation to check that Go code is following unsafe.Pointer safety rules dynamically. The new flag -d=checkptr is enabled by default with the -race or -msan flags.

Specifically, -d=checkptr checks the following:

  1. When converting unsafe.Pointer to *T, the resulting pointer must be aligned appropriately for T.
  2. If the result of pointer arithmetic points into a Go heap object, one of the unsafe.Pointer-typed operands must point into the same object.

So if you run unit test with -race in Go 1.14 you will get a panic like

fatal error: checkptr: unsafe pointer conversion
goroutine 1 [running]:
runtime.throw(0x6bbf63a, 0x23)
	/usr/local/go/src/runtime/panic.go:1112 +0x72 fp=0xc000e0bcf8 sp=0xc000e0bcc8 pc=0x40368e2
runtime.checkptrAlignment(0xc00065f1bf, 0x6981ba0, 0x1)
	/usr/local/go/src/runtime/checkptr.go:13 +0xd0 fp=0xc000e0bd28 sp=0xc000e0bcf8 pc=0x4008710
github.com/pingcap/tidb/kv/memdb.(*DB).newNode(0xc000de28a0, 0xc000de2870, 0xc00005d530, 0x1b, 0x23, 0xc0001b0300, 0x10, 0x10, 0x1, 0x0, ...)
	/Users/tangliu/program/go/pkg/mod/github.com/pingcap/tidb@v1.1.0-beta.0.20200223044457-aedea3ec5e1e/kv/memdb/memdb.go:300 +0xbe fp=0xc000e0bda8 sp=0xc000e0bd28 pc=0x4b3b05e
github.com/pingcap/tidb/kv/memdb.(*DB).Put(0xc000de28a0, 0xc00005d530, 0x1b, 0x23, 0xc0001b0300, 0x10, 0x10, 0x6898b01)
	/Users/tangliu/program/go/pkg/mod/github.com/pingcap/tidb@v1.1.0-beta.0.20200223044457-aedea3ec5e1e/kv/memdb/memdb.go:91 +0x2fb fp=0xc000e0c0a8 sp=0xc000e0bda8 pc=0x4b390bb
github.com/pingcap/tidb/kv.(*memDbBuffer).Set(0xc000da1fc0, 0xc00005d530, 0x1b, 0x23, 0xc0001b0300, 0x10, 0x10, 0x6614180, 0x0)
	/Users/tangliu/program/go/pkg/mod/github.com/pingcap/tidb@v1.1.0-beta.0.20200223044457-aedea3ec5e1e/kv/memdb_buffer.go:104 +0x3fd fp=0xc000e0c1a0 sp=0xc000e0c0a8 pc=0x4b3dcdd
github.com/pingcap/tidb/kv.(*lazyMemBuffer).Set(0xc000bfef00, 0xc00005d530, 0x1b, 0x23, 0xc0001b0300, 0x10, 0x10, 0xaa592f0, 0x0)
	/Users/tangliu/program/go/pkg/mod/github.com/pingcap/tidb@v1.1.0-beta.0.20200223044457-aedea3ec5e1e/kv/union_store.go:141 +0xb5 fp=0xc000e0c208 sp=0xc000e0c1a0 pc=0x4b41535
github.com/pingcap/tidb/kv.(*unionStore).Set(0xc000bfef40, 0xc00005d530, 0x1b, 0x23, 0xc0001b0300, 0x10, 0x10, 0xc0001b030f, 0x10)
	<autogenerated>:1 +0xcd fp=0xc000e0c280 sp=0xc000e0c208 pc=0x4b4403d
github.com/pingcap/tidb/store/tikv.(*tikvTxn).Set(0xc000ad98c0, 0xc00005d530, 0x1b, 0x23, 0xc0001b0300, 0x10, 0x10, 0x0, 0x0)

What is changed and how it works?

In fact, the current implementation does not really cause illegal memory access. The node struct is actually a variable-sized struct, the nexts array is dynamically allocated according to the node's height. But checkptrAlignment will use the size defined by the struct, which is usually larger than the actual size of the object.

As a result, when Go's checker check the memory boundary according to rule 2, it will find some bytes in the node.nexts doesn't in the memory space returned by the allocator.

To address this problem, I modified the definition of node struct, change nexts array to nextsBase. Because each node always has at least one element in nexts, the checker will never fail.

The rule 1 is easy to address, just align the allocation in arenaBlock.

Check List

Tests

  • Manual test (add detailed scripts or steps below)
    • Run test with -race using Go 1.14

@codecov
Copy link

codecov bot commented Feb 27, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@899c274). Click here to learn what that means.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #14972   +/-   ##
===========================================
  Coverage          ?   80.3461%           
===========================================
  Files             ?        502           
  Lines             ?     131455           
  Branches          ?          0           
===========================================
  Hits              ?     105619           
  Misses            ?      17500           
  Partials          ?       8336

@coocood
Copy link
Member

coocood commented Feb 27, 2020

LGTM

@tiancaiamao
Copy link
Contributor

LGTM

Some of my personal projects might also be influenced by Go 1.14, I miss C so much...

@tiancaiamao tiancaiamao added the status/LGT2 Indicates that a PR has LGTM 2. label Feb 27, 2020
@tiancaiamao
Copy link
Contributor

/run-all-tests

@jackysp
Copy link
Member

jackysp commented Feb 27, 2020

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 27, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Feb 27, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Feb 27, 2020

@bobotu merge failed.

@tiancaiamao
Copy link
Contributor

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Feb 27, 2020

Your auto merge job has been accepted, waiting for 14962

@sre-bot
Copy link
Contributor

sre-bot commented Feb 27, 2020

/run-all-tests

@tiancaiamao
Copy link
Contributor

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Feb 28, 2020

/run-all-tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants