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

Fix panic on 32-bit platforms #10515

Merged
merged 3 commits into from
Jun 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/10515.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
agent: fix a panic on 32-bit platforms caused by misaligned struct fields used with sync/atomic.
```
47 changes: 46 additions & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -116,14 +116,20 @@ jobs:
- run: go get -u github.com/hashicorp/lint-consul-retry && lint-consul-retry
- run: *notify-slack-failure

# Runs Go linters
lint:
description: "Run golangci-lint"
parameters:
go-arch:
type: string
default: ""
docker:
- image: *GOLANG_IMAGE
environment:
GOTAGS: "" # No tags for OSS but there are for enterprise
GOARCH: "<<parameters.go-arch>>"
steps:
- checkout
- run: go env
- run:
name: Install golangci-lint
command: |
Expand Down Expand Up @@ -275,6 +281,40 @@ jobs:
path: /tmp/jsonfile
- run: *notify-slack-failure

# go-test-32bit is to catch problems where 64-bit ints must be 64-bit aligned
# to use them with sync/atomic. See https://golang.org/pkg/sync/atomic/#pkg-note-BUG.
# Running tests with GOARCH=386 seems to be the best way to detect this
# problem. Only runs tests that are -short to limit the time we spend checking
# for these bugs.
go-test-32bit:
docker:
- image: *GOLANG_IMAGE
environment:
<<: *ENVIRONMENT
GOTAGS: "" # No tags for OSS but there are for enterprise
steps:
- checkout
- run: *install-gotestsum
- run: go mod download
- run:
name: go test 32-bit
environment:
GOARCH: 386
command: |
mkdir -p $TEST_RESULTS_DIR /tmp/jsonfile
go env
gotestsum \
--jsonfile /tmp/jsonfile/go-test-32bit.log \
--junitfile $TEST_RESULTS_DIR/gotestsum-report.xml -- \
-tags="$GOTAGS" -p 2 \
-short ./...

- store_test_results:
path: *TEST_RESULTS_DIR
- store_artifacts:
path: *TEST_RESULTS_DIR
- run: *notify-slack-failure

go-test-lib:
description: "test a library against a specific Go version"
parameters:
Expand Down Expand Up @@ -959,6 +999,10 @@ workflows:
- check-generated-protobuf: *filter-ignore-non-go-branches
- lint-consul-retry: *filter-ignore-non-go-branches
- lint: *filter-ignore-non-go-branches
- lint:
name: "lint-32bit"
go-arch: "386"
<<: *filter-ignore-non-go-branches
- test-connect-ca-providers: *filter-ignore-non-go-branches
- dev-build: *filter-ignore-non-go-branches
- go-test:
Expand All @@ -984,6 +1028,7 @@ workflows:
go-version: "1.15"
<<: *filter-ignore-non-go-branches
- go-test-race: *filter-ignore-non-go-branches
- go-test-32bit: *filter-ignore-non-go-branches
build-distros:
unless: << pipeline.parameters.trigger-load-test >>
jobs:
Expand Down
4 changes: 2 additions & 2 deletions agent/config/runtime_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5473,7 +5473,7 @@ func TestLoad_FullConfig(t *testing.T) {
HTTPSPort: 15127,
HTTPUseCache: false,
KeyFile: "IEkkwgIA",
KVMaxValueSize: 1234567800000000,
KVMaxValueSize: 1234567800,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about why we changed to use a 32 bit value in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! I left some detail in the commit message for f34d354

The hcl decoding apparently uses strconv.ParseInt, which fails to parse a 64bit int on a 32bit platform. Since hcl v1 is basically EOL, it seems unlikely we'll fix this in hcl. We run these tests with both json and hcl input, and only the hcl input was failing.

Since this test is only about loading values from config files, the extra large number doesn't seem important, so I trimmed the number a bit to fit into 32bit.

LeaveDrainTime: 8265 * time.Second,
LeaveOnTerm: true,
Logging: logging.Config{
Expand Down Expand Up @@ -5868,7 +5868,7 @@ func TestLoad_FullConfig(t *testing.T) {
"wan_ipv4": "78.63.37.19",
},
TranslateWANAddrs: true,
TxnMaxReqLen: 5678000000000000,
TxnMaxReqLen: 567800000,
UIConfig: UIConfig{
Dir: "pVncV4Ey",
ContentPath: "/qp1WRhYH/", // slashes are added in parsing
Expand Down
4 changes: 2 additions & 2 deletions agent/config/testdata/full-config.hcl
Original file line number Diff line number Diff line change
Expand Up @@ -285,8 +285,8 @@ limits {
rpc_rate = 12029.43
rpc_max_burst = 44848
rpc_max_conns_per_client = 2954
kv_max_value_size = 1234567800000000
txn_max_req_len = 5678000000000000
kv_max_value_size = 1234567800
txn_max_req_len = 567800000
}
log_level = "k1zo9Spt"
log_json = true
Expand Down
4 changes: 2 additions & 2 deletions agent/config/testdata/full-config.json
Original file line number Diff line number Diff line change
Expand Up @@ -285,8 +285,8 @@
"rpc_rate": 12029.43,
"rpc_max_burst": 44848,
"rpc_max_conns_per_client": 2954,
"kv_max_value_size": 1234567800000000,
"txn_max_req_len": 5678000000000000
"kv_max_value_size": 1234567800,
"txn_max_req_len": 567800000
},
"log_level": "k1zo9Spt",
"log_json": true,
Expand Down
12 changes: 8 additions & 4 deletions agent/grpc/stats.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,11 @@ var defaultMetrics = metrics.Default
// statsHandler is a grpc/stats.StatsHandler which emits connection and
// request metrics to go-metrics.
type statsHandler struct {
// activeConns is used with sync/atomic and MUST be 64-bit aligned. To ensure
// alignment on 32-bit platforms this field must remain the first field in
// the struct. See https://golang.org/pkg/sync/atomic/#pkg-note-BUG.
activeConns uint64
metrics *metrics.Metrics
activeConns uint64 // must be 8-byte aligned for atomic access
}

func newStatsHandler(m *metrics.Metrics) *statsHandler {
Expand Down Expand Up @@ -103,10 +106,11 @@ func (c *statsHandler) HandleConn(_ context.Context, s stats.ConnStats) {
}

type activeStreamCounter struct {
// count is used with sync/atomic and MUST be 64-bit aligned. To ensure
// alignment on 32-bit platforms this field must remain the first field in
// the struct. See https://golang.org/pkg/sync/atomic/#pkg-note-BUG.
count uint64
metrics *metrics.Metrics
// count of the number of open streaming RPCs on a server. It is accessed
// atomically.
count uint64
}

// GRPCCountingStreamInterceptor is a grpc.ServerStreamInterceptor that emits a
Expand Down
4 changes: 4 additions & 0 deletions command/connect/redirecttraffic/redirect_traffic_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ func TestRun_FlagValidation(t *testing.T) {
}

func TestGenerateConfigFromFlags(t *testing.T) {
if testing.Short() {
t.Skip("too slow for testing.Short")
}

cases := []struct {
name string
command func() cmd
Expand Down
8 changes: 5 additions & 3 deletions tlsutil/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,11 @@ type manual struct {
// Configurator receives an initial TLS configuration from agent configuration,
// and receives updates from config reloads, auto-encrypt, and auto-config.
type Configurator struct {
// version is increased each time the Configurator is updated. Must be accessed
// using sync/atomic. Also MUST be the first field in this struct to ensure
// 64-bit alignment. See https://golang.org/pkg/sync/atomic/#pkg-note-BUG.
version uint64

// lock synchronizes access to all fields on this struct except for logger and version.
lock sync.RWMutex
base *Config
Expand All @@ -188,9 +193,6 @@ type Configurator struct {
// logger is not protected by a lock. It must never be changed after
// Configurator is created.
logger hclog.Logger
// version is increased each time the Configurator is updated. Must be accessed
// using sync/atomic.
version uint64
}

// NewConfigurator creates a new Configurator and sets the provided
Expand Down