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

Patch unaligned 64-bit atomic operation panic #37

Closed
wants to merge 1 commit into from
Closed

Patch unaligned 64-bit atomic operation panic #37

wants to merge 1 commit into from

Conversation

silentmurdock
Copy link

Fix unaligned 64-bit atomic operation panic to support 32-bit architectures too.

@anacrolix
Copy link
Owner

Damn, does it really take that much?!

@anacrolix
Copy link
Owner

Was moving the stat block to the beginning of the struct not enough? What if it's detached and pointed to instead of embedded?

@silentmurdock
Copy link
Author

You're right, I looked through the code a little more and found that if i move the "traversal" block to the beginning of "Announce" struct in "announce.go" that also solves the problem. Working on 32-bit systems.

I am revoking this pull request.
Please modify the code.

@silentmurdock
Copy link
Author

A simpler solution is also available.

@silentmurdock silentmurdock deleted the patch-torrent-issue-483 branch May 25, 2021 04:41
@anacrolix
Copy link
Owner

anacrolix commented May 25, 2021

@silentmurdock I commented about this in anacrolix/torrent#483 (comment) but got feedback to the contrary from @compliment . Can you confirm the patch in the linked comment fix this for you on the platforms you're using?

@compliment
Copy link

compliment commented May 25, 2021

You're right, I looked through the code a little more and found that if i move the "traversal" block to the beginning of "Announce" struct in "announce.go" that also solves the problem. Working on 32-bit systems.

This is fd60ad0
I tried it with with https://github.com/anacrolix/torrent/tree/master/cmd/torrent for linux 386 and got

panic: unaligned 64-bit atomic operation

goroutine 795 [running]:
runtime/internal/atomic.panicUnaligned()
	runtime/internal/atomic/unaligned.go:8 +0x2d
runtime/internal/atomic.Xadd64(0x9e321e4, 0x1, 0x0, 0x0, 0x0)
	runtime/internal/atomic/asm_386.s:107 +0x11
github.com/anacrolix/dht/v2.(*traversal).wrapQuery(0x9e321e4, 0x86c4250, 0x9c753c0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	github.com/anacrolix/dht/v2@v2.9.2-0.20210509011734-fd60ad06b482/traversal.go:115 +0x52
github.com/anacrolix/dht/v2.(*traversal).run.func1.1(0x9c0e008)
	github.com/anacrolix/dht/v2@v2.9.2-0.20210509011734-fd60ad06b482/traversal.go:156 +0x35
github.com/anacrolix/dht/v2.(*traversal).beginQuery.func1.1(0x0)
	github.com/anacrolix/dht/v2@v2.9.2-0.20210509011734-fd60ad06b482/traversal.go:179 +0x93
github.com/anacrolix/dht/v2.(*Server).beginQuery.func1.1()
	github.com/anacrolix/dht/v2@v2.9.2-0.20210509011734-fd60ad06b482/server.go:745 +0x23
created by github.com/anacrolix/dht/v2.(*traversal).run
	github.com/anacrolix/dht/v2@v2.9.2-0.20210509011734-fd60ad06b482/traversal.go:164 +0x83 

Are you sure this worked for you and you didn't have your own patch applied?

@anacrolix
Copy link
Owner

Thanks for chasing this up. I'll follow this up when you guys determine the best way forward.

@silentmurdock silentmurdock restored the patch-torrent-issue-483 branch May 25, 2021 12:19
@silentmurdock silentmurdock deleted the patch-torrent-issue-483 branch May 25, 2021 12:26
@silentmurdock
Copy link
Author

silentmurdock commented May 25, 2021

Are you sure this worked for you and you didn't have your own patch applied?

The fd60ad0 didn’t work for me either, I used my own patch solution first: torrent-issue-483_solution_2.zip

Anacrolix's solution didn't work before, because he modified only the "traversal.go" file, but "announce.go" has an other struct that need to be modified. I extended his solution, so it works now: torrent-issue-483_solution_1.zip

Both versions work, but I think Anacrolix's modified solution is simpler and faster.
Please write over the specified files and test it.

Which one do we use?

@anacrolix
Copy link
Owner

I think mine is simpler. It would be great if this was checked in CI. It's recently moved to GitHub actions if you're keen/know how to implement that.

@silentmurdock
Copy link
Author

silentmurdock commented May 26, 2021

Unfortunately, CircleCI does not contain 32-bit images. I don’t feel so confident in using CI. Maybe if we compile the 32-bit code on a 64-bit system image and run the tests there. (Example - run: GOOS = linux GOARCH = arm GOARM = 6)

@silentmurdock
Copy link
Author

@anacrolix Can you add the changes because my app doesn't work without it? I don’t want to use a vendor library in my project. I tested it manually on arm, linux, windows 32 bit systems. I will be working on a CI test solution in the future.
Or do I restore the branch, modify, and create a new pull requests?

boypt pushed a commit to boypt/dht that referenced this pull request Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants