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

Runtime panic caused by unaligned memory in arm32 architectures #2336

Closed
jacquesadit opened this issue Mar 19, 2021 · 0 comments
Closed

Runtime panic caused by unaligned memory in arm32 architectures #2336

jacquesadit opened this issue Mar 19, 2021 · 0 comments
Labels

Comments

@jacquesadit
Copy link
Contributor

jacquesadit commented Mar 19, 2021

Description

The following line causes a runtime panic on 32bit arm architectures.

return atomic.AddUint64(&p.ReqID, 1)

Cause

A well known requirement of atomic.AddUint64 is that the address must be 64 bit aligned on 32bit arm architectures. This was picked up by the go-staticcheck linter, see https://staticcheck.io/docs/checks#SA1027 and this issue is also described in this article https://go101.org/article/memory-layout.html in particular the section The Alignment Requirement for 64-bit Word Atomic Operations. In our case the ReqID field in MessageServer comes after the log.log field and so is not guaranteed to be 8 byte aligned. A proposal exist in golang for the addresses of 64-bit words to be always be 8-byte aligned but has not yet been implemented, see: golang/go#36606.

Proposed Solution

Move ReqID to the top of MessageServer to guarantee 8 byte alignment. Testing with this change fixes the issue for me.

Steps To Reproduce

  1. Follow the normal build instructions on a Raspberry Pi 4B or (other 32bit arm computer with armv7l architecture).
  2. Run go-spacemesh --config <path to config.json> with the config file found here: https://storage.googleapis.com/spacecraft-data/tweedlelite126-archive/config.json
  3. Wait approx 20 seconds until a p2p SendRequest is made at which point the following panic occurs:
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x12668]

goroutine 211 [running]:
runtime/internal/atomic.goXadd64(0x212a124, 0x1, 0x0, 0xca2790, 0x99a3d0)
        /usr/lib/go-1.14/src/runtime/internal/atomic/atomic_arm.go:103 +0x1c
github.com/spacemeshos/go-spacemesh/p2p/server.(*MessageServer).newReqID(...)
        /home/pi/go-spacemesh/p2p/server/msgserver.go:251
github.com/spacemeshos/go-spacemesh/p2p/server.(*MessageServer).SendRequest(0x212a120, 0x0, 0x2043a80, 0x3c, 0x40, 0xc99460, 0x20c5180, 0x210b0c0, 0xb1daec, 0x9c8200, ...)
        /home/pi/go-spacemesh/p2p/server/msgserver.go:231 +0x3c
github.com/spacemeshos/go-spacemesh/p2p/discovery.(*protocol).Ping(0x23c93c0, 0xc99460, 0x20c5180, 0x20c5180, 0x257c9f0)
        /home/pi/go-spacemesh/p2p/discovery/ping.go:91 +0x278
github.com/spacemeshos/go-spacemesh/p2p/discovery.pingThenGetAddresses(0xa477e0d0, 0x23c93c0, 0x237c240, 0x257cb40)
        /home/pi/go-spacemesh/p2p/discovery/refresher.go:154 +0xbc
created by github.com/spacemeshos/go-spacemesh/p2p/discovery.(*refresher).requestAddresses
        /home/pi/go-spacemesh/p2p/discovery/refresher.go:203 +0x3cc

Expected Behavior

No panic.

Environment

  • OS: Ubuntu Server 20.10 on armv7l architecture (Raspberry Pi 4B)

Tested on node versions:

  • v0.1.26+ce1d13ba5358d1d507dac817f0d385208cd65d13
  • v0.0.0-unreleased+2671a2491609be5952149979e9983e3e5b68dbb2
bors bot pushed a commit that referenced this issue Jul 13, 2021
Atomic operations such as atomic.AddUint64 must operate on 64-bit aligned memory when running on 32-bit systems, otherwise a runtime will panic occur. This is a known issue in go and might be addressed in a future release, see golang/go#36606.

## Motivation
Using atomic.AddUint64 on non 64 bit aligned address on 32 bit systems results in a runtime panic. The solution is to guarantee the data it operates on is 64 bit aligned. This can be done by defining such fields as the first item in a struct. See #2336.

Closes #2336 

## Changes
Moving ReqID to the top of MessageServer guarantees 8 byte alignment. See https://go101.org/article/memory-layout.html section _The Alignment Requirement for 64-bit Word Atomic Operations_ for details on why this works.

## Test Plan
This was tested on a Raspberry Pi 4B (armv7l) and on linux x86_64 resulting in no breaking changes. 

## TODO
- [ ] Test changes

## DevOps Notes
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [x] This PR does not affect public APIs
- [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
- [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)


Co-authored-by: Nikita Kryuchkov <nkryuchkov10@gmail.com>
bors bot pushed a commit that referenced this issue Jul 13, 2021
Atomic operations such as atomic.AddUint64 must operate on 64-bit aligned memory when running on 32-bit systems, otherwise a runtime will panic occur. This is a known issue in go and might be addressed in a future release, see golang/go#36606.

## Motivation
Using atomic.AddUint64 on non 64 bit aligned address on 32 bit systems results in a runtime panic. The solution is to guarantee the data it operates on is 64 bit aligned. This can be done by defining such fields as the first item in a struct. See #2336.

Closes #2336 

## Changes
Moving ReqID to the top of MessageServer guarantees 8 byte alignment. See https://go101.org/article/memory-layout.html section _The Alignment Requirement for 64-bit Word Atomic Operations_ for details on why this works.

## Test Plan
This was tested on a Raspberry Pi 4B (armv7l) and on linux x86_64 resulting in no breaking changes. 

## TODO
- [ ] Test changes

## DevOps Notes
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [x] This PR does not affect public APIs
- [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
- [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)


Co-authored-by: Nikita Kryuchkov <nkryuchkov10@gmail.com>
bors bot pushed a commit that referenced this issue Jul 13, 2021
Atomic operations such as atomic.AddUint64 must operate on 64-bit aligned memory when running on 32-bit systems, otherwise a runtime will panic occur. This is a known issue in go and might be addressed in a future release, see golang/go#36606.

## Motivation
Using atomic.AddUint64 on non 64 bit aligned address on 32 bit systems results in a runtime panic. The solution is to guarantee the data it operates on is 64 bit aligned. This can be done by defining such fields as the first item in a struct. See #2336.

Closes #2336 

## Changes
Moving ReqID to the top of MessageServer guarantees 8 byte alignment. See https://go101.org/article/memory-layout.html section _The Alignment Requirement for 64-bit Word Atomic Operations_ for details on why this works.

## Test Plan
This was tested on a Raspberry Pi 4B (armv7l) and on linux x86_64 resulting in no breaking changes. 

## TODO
- [ ] Test changes

## DevOps Notes
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [x] This PR does not affect public APIs
- [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
- [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)


Co-authored-by: Nikita Kryuchkov <nkryuchkov10@gmail.com>
@bors bors bot closed this as completed in 663ebe1 Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant