Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix atomic access to unaligned fields causing panics on 32bit systems #35

Merged
merged 1 commit into from
Feb 21, 2022

Conversation

kumbayo
Copy link
Contributor

@kumbayo kumbayo commented Jan 26, 2022

We access the "written" and "entries" fields in "Archiver" and "Extractor"
using atomic operations.
This operations require the fields to be 8 byte aligned.
Otherwise they panic at runtime.

On 32bit systems, int64 fields are not necessarily aligned to 8 bytes.
They may just be aligned to 4 bytes depending on the fields before them.

This currently causes fastzip to reproducably fail archiving and extraction
on 32bit systems.

Move the fields which are accessed via atomic operations
to the start of the struct to properly align them and avoid the problem.

See
https://go101.org/article/memory-layout.html
"The Alignment Requirement for 64-bit Word Atomic Operations"
However, on 32-bit architectures, the alignment guarantee
made by the standard Go compiler for 64-bit words is only 4 bytes.
64-bit atomic operations on a 64-bit word which is not 8-byte aligned
will panic at runtime.
...
The ways are described as the first (64-bit) word in a variable
or in an allocated struct, array, or slice can be relied upon to be 64-bit aligned.

See
https://pkg.go.dev/sync/atomic#pkg-note-BUG
On ARM, 386, and 32-bit MIPS, it is the caller's responsibility
to arrange for 64-bit alignment of 64-bit words accessed atomically.
The first word in a variable or in an allocated struct, array,
or slice can be relied upon to be 64-bit aligned.

Related
golang/go#11891

The panic behavior can easily be reproduced by running "go test" in fastzip
on Windows with go 1.17.6 i386 installed.

panic({0xca2aa0, 0xd0fa90})
        C:/Program Files (x86)/Go/src/runtime/panic.go:1038 +0x1bc
runtime/internal/atomic.panicUnaligned()
        C:/Program Files (x86)/Go/src/runtime/internal/atomic/unaligned.go:8 +0x2d
runtime/internal/atomic.Load64(0x11ce0084)
        C:/Program Files (x86)/Go/src/runtime/internal/atomic/atomic_386.s:225 +0x10
github.com/saracen/fastzip.(*Archiver).Written(0x11ce0050)
        C:/go/fastzip/archiver.go:94 +0x26
github.com/saracen/fastzip.TestArchiveCancelContext(0x11c84960)
        C:/go/fastzip/archiver_test.go:165 +0x506

We access the "written" and "entries" fields in "Archiver" and "Extractor"
using atomic operations.
This operations require the fields to be 8 byte aligned.
Otherwise they panic at runtime.

On 32bit systems, int64 fields are not necessarily aligned to 8 bytes.
They may just be aligned to 4 bytes depending on the fields before them.

This currently causes fastzip to reproducably fail archiving and extraction
on 32bit systems.

Move the fields which are accessed via atomic operations
to the start of the struct to properly align them and avoid the problem.

See
https://go101.org/article/memory-layout.html
"The Alignment Requirement for 64-bit Word Atomic Operations"
However, on 32-bit architectures, the alignment guarantee
made by the standard Go compiler for 64-bit words is only 4 bytes.
64-bit atomic operations on a 64-bit word which is not 8-byte aligned
will panic at runtime.
...
The ways are described as the first (64-bit) word in a variable
or in an allocated struct, array, or slice can be relied upon to be 64-bit aligned.

See
https://pkg.go.dev/sync/atomic#pkg-note-BUG
On ARM, 386, and 32-bit MIPS, it is the caller's responsibility
to arrange for 64-bit alignment of 64-bit words accessed atomically.
The first word in a variable or in an allocated struct, array,
or slice can be relied upon to be 64-bit aligned.

Related
golang/go#11891

The panic behavior can easily be reproduced by running "go test" in fastzip
on Windows with go 1.17.6 i386 installed.
panic({0xca2aa0, 0xd0fa90})
        C:/Program Files (x86)/Go/src/runtime/panic.go:1038 +0x1bc
runtime/internal/atomic.panicUnaligned()
        C:/Program Files (x86)/Go/src/runtime/internal/atomic/unaligned.go:8 +0x2d
runtime/internal/atomic.Load64(0x11ce0084)
        C:/Program Files (x86)/Go/src/runtime/internal/atomic/atomic_386.s:225 +0x10
github.com/saracen/fastzip.(*Archiver).Written(0x11ce0050)
        C:/go/fastzip/archiver.go:94 +0x26
github.com/saracen/fastzip.TestArchiveCancelContext(0x11c84960)
        C:/go/fastzip/archiver_test.go:165 +0x506
@kumbayo
Copy link
Contributor Author

kumbayo commented Jan 26, 2022

I noticed this problem when trying to use a 32bit gitlab-runner with fastzip enabled via FF_USE_FASTZIP=true
https://docs.gitlab.com/runner/configuration/feature-flags.html#available-feature-flags

@saracen
Copy link
Owner

saracen commented Feb 21, 2022

@kumbayo Thank you for this!

@saracen saracen merged commit 468caac into saracen:master Feb 21, 2022
@kumbayo kumbayo deleted the fix_atomic_access_on_32bit_systems branch February 22, 2022 17:34
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants