-
Notifications
You must be signed in to change notification settings - Fork 87
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
Android Q Beta - Wrong Bionic TLS alignment (golang) #370
Comments
Let's wait until Q is finished like I wrote on the other ticket in official... Maybe google fixes this before release. |
Sounds like I've made a big mistake by updating to Android Q Beta :( |
I'm sorry to hear that. Probably there's also bad news as Google is about to launch Fuchsia OS and I don't know if this will kill Syncthing for Android or if there's a way to continue it then. |
Topic question: Offtopic: |
It might also be a go or ndk issue... |
A small, but good update (at least for me :) ... ): First of all you should not wait for Android Q final - this will be not fixed by android itself. This is golang issue with bionic wrong tls (thread local storage) memory layout assumption. This should be fixed in golang, but probably bionic's devs may help with this. This is a bad side of a story. The good one is that this may be fixed by a dirty hack for arm and arm64 for now. Android crashes on library load with message of underaligned tls segment, but it seems that at least for arm and arm64 golang is unaware of this segment. I've tried to remove or realign this section with go build, but there was no such option... So I've decided to make a really dirty hack. I've patched .so files for arm/arm64 (arm64 is used on pixel 3). I've changed only one byte from 0x08 to 0x40 - PT_TLS section alignment, which is really unused. And it works now. For arm64 the offset will be 0x1c0 Offsets were calculated according to readelf tool info and Elf32/Elf64 specification. Anyway I'm happy with working syncthing on my Android Q beta now! |
How can that patch be applied automatically by a command line build script?! |
And that's where the story ends for me. Android Q will be released somewhere around August which is about the same timewindow for the next golang release. |
@Catfriend1 , I will write python script today. |
@otbutz , seems so, but hacky patch isn't harmfull - it just changes TLS segment algnment. Nothing will change for regular users and beta users will be happy :) |
Python is very good because that's the Lang of our build script |
@kvaster You might want to use code blocks to wrap your log output so you don't mention issues |
@kvaster What are the correct values for armeabi (arm32) and x86 so I could test on the emulator? |
What command line did you use with the readelf? Where to look? |
Do you mean: |
As I cant get anything to work, would you please upload your libsyncthing.so before and after the patching? |
I cant get this verified if the PR works, somewhere read about Android Studio does not provide arm64-v8a images for Android Q with pixel 3 yet. x86 was discussed in the TLS alignment threads as "wont be supported any longer" but I only have that emulator. |
Tried to compile for x86 with -mstackrealign and -mno-stackrealign with no luck. |
Still getting this on the x86 emulator:
```
2019-03-17 01:41:52.063 11695-11695/? A/libc: Fatal signal 6 (SIGABRT), code -1 (SI_QUEUE) in tid 11695 (libsyncthing.so), pid 11695 (libsyncthing.so)
2019-03-17 01:41:52.077 11698-11698/? A/DEBUG: *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
2019-03-17 01:41:52.077 11698-11698/? A/DEBUG: Build fingerprint: 'google/sdk_gphone_x86/generic_x86:Q/QPP1.190205.018.B3/5345599:userdebug/dev-keys'
2019-03-17 01:41:52.078 11698-11698/? A/DEBUG: Revision: '0'
2019-03-17 01:41:52.078 11698-11698/? A/DEBUG: ABI: 'x86'
2019-03-17 01:41:52.109 11698-11698/? A/DEBUG: Timestamp: 2019-03-17 00:41:52+0000
2019-03-17 01:41:52.109 11698-11698/? A/DEBUG: pid: 11695, tid: 11695, name: libsyncthing.so >>> /data/app/com.github.catfriend1.syncthingandroid.debug-tocqgDB26hUmjeKwRPDoCA==/lib/x86/libsyncthing.so <<<
2019-03-17 01:41:52.109 11698-11698/? A/DEBUG: signal 6 (SIGABRT), code -1 (SI_QUEUE), fault addr --------
2019-03-17 01:41:52.109 11698-11698/? A/DEBUG: eax 00000000 ebx 00002daf ecx 00002daf edx 00000006
2019-03-17 01:41:52.109 11698-11698/? A/DEBUG: edi f6f8a02e esi ff85afc0
2019-03-17 01:41:52.109 11698-11698/? A/DEBUG: ebp f7241b30 esp ff85af68 eip f7241b39
2019-03-17 01:41:52.111 11698-11698/? A/DEBUG: backtrace:
2019-03-17 01:41:52.112 11698-11698/? A/DEBUG: #00 pc 00000b39 [vdso] (__kernel_vsyscall+9)
2019-03-17 01:41:52.112 11698-11698/? A/DEBUG: #1 pc 000941e8 /bionic/lib/libc.so (syscall+40)
2019-03-17 01:41:52.112 11698-11698/? A/DEBUG: #2 pc 000af461 /bionic/lib/libc.so (abort+193)
2019-03-17 01:41:52.112 11698-11698/? A/DEBUG: #3 pc 0074048a /data/app/com.github.catfriend1.syncthingandroid.debug-tocqgDB26hUmjeKwRPDoCA==/lib/x86/libsyncthing.so (inittls+170)
2019-03-17 01:41:52.121 1731-1731/? E//system/bin/tombstoned: Tombstone written to: /data/tombstones/tombstone_12
```
|
Python script for patching alignment: https://pastebin.com/XZMKMmMd |
Thanks for your contributon. It doesn't work on x86. Your code is far better than mine because you dynamically determine which byte to change. I've already found out yesterday by try & readelf that your above mentioned positions of the byte aren't static across builds (had 0x150 instead of 0x14c for example). I've incorporated your script into the native build script. Builds fine, but now I need feedback from armeabi and arm64-v8a device testers before releasing it. Here's the new build log.
|
See release page. https://github.com/Catfriend1/syncthing-android/releases |
@NatoBoram, I was using code blocks. Seems that it was due to missing emtpy line after tag. Not my fault. |
For x86, the alignment of the TLS segment isn't an issue. It's only a problem on arm32/arm64, because the normal ELF TLS ABI allocates an executable's TLS memory in a way that overlaps with Bionic's existing TLS slots (TLS_SLOT_xxx). To avoid the conflict, an arm32/arm64 executable's TLS segment needs at least a 32-byte alignment on arm32 and a 64-byte alignment on arm64. FWIW: I think the binary patching fix will work for Golang-on-Android, but it's not a general fix for the |
@rprichard: Thanks for the useful insights. What I don't understand is why tls alignment isn't an issue on x86 but on android q x86 emulator from android studio I'm getting the crash on inittls()+189. See #370 (comment) |
@jaseemabid does syncthing fork work on your android q device? |
@Catfriend1 I'vent tried the fork yet. I'll give that a try soon. Is it possible to get the patch upstream and make a new release or do you want more of us to test this out? |
I think the dirty fix will not suit upstream requirements, but if many people say it's okay including to add their Android make model and versions we could have a proposal to upstream. |
+1 your patch fixes the issue on my pixel2 |
+1 your patch fixes the issue on my pixel3xl |
Does this apk also work on your q phones? It's including an official patch on the go side instead of the binary quick win. |
works for me @Catfriend1 🎉 |
@danblah ? |
@Catfriend1 Works for me on Pixel 2 XL with Q Beta 2. |
Works like a charm, Thank you ! Oneplus 6 A6003 Android Q DP2 |
Thank you very much for the provided feedback. :-) |
- Update .travis.yml (Bump go, NDK) - Remove dirty patch of the underaligned TLS segment in libSyncthing.so as this is officially solved by Go 1.13 (ref: ##370)
@Athanasius well, here's a version to fill the time until release.... |
Syncthing crashes on Pixel 3 XL after upgrading to Android Q Beta
The text was updated successfully, but these errors were encountered: