-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Conversation
sync/atomic must be used with 64-bit aligned fields, and that alignment is difficult to ensure unless the field is the first one in the struct. https://golang.org/pkg/sync/atomic/#pkg-note-BUG.
The hcl decoding apparently uses strconv.ParseInt, which fails to parse a 64bit int. Since hcl v1 is basically EOl, it seems unlikely we'll fix this in hcl. Since this test is only about loading values from config files, the extra large number doesn't seem important. Trim a few zeros from the numbers so that they parse properly on 32bit platforms. Also skip a slow test when -short is used.
86a5aff
to
3b04326
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other then a quick question it LGTM
@@ -5473,7 +5473,7 @@ func TestLoad_FullConfig(t *testing.T) { | |||
HTTPSPort: 15127, | |||
HTTPUseCache: false, | |||
KeyFile: "IEkkwgIA", | |||
KVMaxValueSize: 1234567800000000, | |||
KVMaxValueSize: 1234567800, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/401754. |
…gment Fix panic on 32-bit platforms Conflicts in tlsutil/config.go were resolved by dropping those changes. The issue that was fixed in that file is not in 1.10.x.
Fixes #10509
Best viewed by individual commit.
Fixes 3 places where a
uint64
struct field was used withsync/atomic
. The fields were not the first field in the struct, and would cause a panic on 32-bit platforms because the other fields caused theuint64
fields to be not 64-bit aligned (https://golang.org/pkg/sync/atomic/#pkg-note-BUG). Also updates the comments to be more explicit about the problem.Adds a new
go-test-32bit
CI job to catch the problem. It only runs-short
tests so that we don't have to run a bunch of parallel jobs, but this seems to catch our existing problems even without running all tests. Hopefully all use ofsync/atomic
is covered by at least one test that runs in under 100ms.TODO: