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

Android Q Beta - Wrong Bionic TLS alignment (golang) #370

Closed
kvaster opened this issue Mar 15, 2019 · 39 comments · Fixed by #374
Closed

Android Q Beta - Wrong Bionic TLS alignment (golang) #370

kvaster opened this issue Mar 15, 2019 · 39 comments · Fixed by #374
Assignees
Labels
build testing Community test case - Notice: Consists of maybe unstable syncthing version or code upstream
Milestone

Comments

@kvaster
Copy link

kvaster commented Mar 15, 2019

Syncthing crashes on Pixel 3 XL after upgrading to Android Q Beta

App Version: 1.1.0.3
Syncthing Version: v1.1.0
Android Version: Android Q
Device manufacturer: Google
Device model: Pixel XL 3
--------- beginning of crash
03-15 11:39:37.094 F/libc    (11681): error: "/data/app/com.github.catfriend1.syncthingandroid-3mY72xEAJsVHsCVYH8zyLA==/lib/arm64/libsyncthing.so": executable's TLS segment is underaligned: alignment is 8, needs to be at least 64 for ARM64 Bionic
03-15 11:39:37.094 F/libc    (11681): Fatal signal 6 (SIGABRT), code -1 (SI_QUEUE) in tid 11681 (libsyncthing.so), pid 11681 (libsyncthing.so)
03-15 11:39:37.123 F/DEBUG   (11688): *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
03-15 11:39:37.123 F/DEBUG   (11688): Build fingerprint: 'google/crosshatch/crosshatch:Q/QPP1.190205.018.B4/5347935:user/release-keys'
03-15 11:39:37.123 F/DEBUG   (11688): Revision: 'MP1.0'
03-15 11:39:37.123 F/DEBUG   (11688): ABI: 'arm64'
03-15 11:39:37.124 F/DEBUG   (11688): Timestamp: 2019-03-15 11:39:37+0300
03-15 11:39:37.124 F/DEBUG   (11688): pid: 11681, tid: 11681, name: libsyncthing.so  >>> /data/app/com.github.catfriend1.syncthingandroid-3mY72xEAJsVHsCVYH8zyLA==/lib/arm64/libsyncthing.so <<<
03-15 11:39:37.124 F/DEBUG   (11688): signal 6 (SIGABRT), code -1 (SI_QUEUE), fault addr --------
03-15 11:39:37.124 F/DEBUG   (11688): Abort message: 'error: "/data/app/com.github.catfriend1.syncthingandroid-3mY72xEAJsVHsCVYH8zyLA==/lib/arm64/libsyncthing.so": executable's TLS segment is underaligned: alignment is 8, needs to be at least 64 for ARM64 Bionic'
03-15 11:39:37.124 F/DEBUG   (11688):     x0  0000000000000000  x1  0000000000002da1  x2  0000000000000006  x3  0000007ffc3a9a40
03-15 11:39:37.124 F/DEBUG   (11688):     x4  8000000000808080  x5  8000000000808080  x6  8000000000808080  x7  0000000000000008
03-15 11:39:37.124 F/DEBUG   (11688):     x8  00000000000000f0  x9  0000007ec8b69580  x10 0000000000000000  x11 0000000000000001
03-15 11:39:37.124 F/DEBUG   (11688):     x12 0000000000000000  x13 0000000000000018  x14 000000009cff6f6d  x15 000562907ab80995
03-15 11:39:37.124 F/DEBUG   (11688):     x16 000000000001191c  x17 0000000000000000  x18 0000007ec8b40000  x19 00000000000000ac
03-15 11:39:37.124 F/DEBUG   (11688):     x20 0000000000002da1  x21 00000000000000b2  x22 0000000000002da1  x23 00000000ffffffff
03-15 11:39:37.124 F/DEBUG   (11688):     x24 0000007ec8b52000  x25 0000007ec8c665a0  x26 0000007ec8c66990  x27 0000007ec8c66000
03-15 11:39:37.124 F/DEBUG   (11688):     x28 0000007ec8c66000  x29 0000007ffc3a9af0
03-15 11:39:37.124 F/DEBUG   (11688):     sp  0000007ffc3a9a20  lr  0000007ec8c31910  pc  0000007ec8c31940
03-15 11:39:37.129 F/DEBUG   (11688): 
03-15 11:39:37.129 F/DEBUG   (11688): backtrace:
03-15 11:39:37.129 F/DEBUG   (11688):       #00 pc 00000000000dd940  /bionic/bin/linker64 (__dl_abort+176)
03-15 11:39:37.129 F/DEBUG   (11688):       #01 pc 00000000000fe47c  /bionic/bin/linker64 (__dl__ZN15StaticTlsLayout27reserve_exe_segment_and_tcbEPK10TlsSegmentPKc+276)
03-15 11:39:37.129 F/DEBUG   (11688):       #02 pc 00000000000504b4  /bionic/bin/linker64 (__dl__Z27linker_setup_exe_static_tlsPKc+68)
03-15 11:39:37.129 F/DEBUG   (11688):       #03 pc 000000000004b4c4  /bionic/bin/linker64 (__dl__ZL29__linker_init_post_relocationR19KernelArgumentBlockR6soinfo+3012)
03-15 11:39:37.129 F/DEBUG   (11688):       #04 pc 000000000004a7f0  /bionic/bin/linker64 (__dl___linker_init+416)
03-15 11:39:37.129 F/DEBUG   (11688):       #05 pc 0000000000051b24  /bionic/bin/linker64 (__dl__start+4)
--------- beginning of main
03-15 11:46:13.218 W/ActivityThread(11599): handleWindowVisibility: no activity for token android.os.BinderProxy@d1805f8
03-15 11:46:13.320 W/sh      (19192): type=1400 audit(0.0:3008): avc: denied { read } for name="/" dev="dm-3" ino=2 scontext=u:r:untrusted_app_27:s0:c512,c768 tcontext=u:object_r:rootfs:s0 tclass=dir permissive=0
03-15 11:46:13.342 I/Util    (11599): runShellCommandGetOutput: Exited with code 0
03-15 11:46:13.359 I/Adreno  (11599): QUALCOMM build                   : a717a27, I40aa93043f
03-15 11:46:13.359 I/Adreno  (11599): Build Date                       : 01/22/19
03-15 11:46:13.359 I/Adreno  (11599): OpenGL ES Shader Compiler Version: EV031.25.19.01
03-15 11:46:13.359 I/Adreno  (11599): Local Branch                     : gfx-adreno.lnx.1.0
03-15 11:46:13.359 I/Adreno  (11599): Remote Branch                    : quic/gfx-adreno.lnx.1.0
03-15 11:46:13.359 I/Adreno  (11599): Remote Branch                    : NONE
03-15 11:46:13.359 I/Adreno  (11599): Reconstruct Branch               : NOTHING
03-15 11:46:13.359 I/Adreno  (11599): Build Config                     : S P 6.0.7 AArch64
03-15 11:46:13.363 I/Adreno  (11599): PFP: 0x016ee180, ME: 0x00000000
03-15 11:46:13.367 I/ConfigStore(11599): android::hardware::configstore::V1_0::ISurfaceFlingerConfigs::hasWideColorDisplay retrieved: 1
03-15 11:46:13.368 I/ConfigStore(11599): android::hardware::configstore::V1_0::ISurfaceFlingerConfigs::hasHDRDisplay retrieved: 1
03-15 11:46:13.394 W/RenderThread(11599): type=1400 audit(0.0:3009): avc: denied { read } for name="u:object_r:vendor_default_prop:s0" dev="tmpfs" ino=25027 scontext=u:r:untrusted_app_27:s0:c512,c768 tcontext=u:object_r:vendor_default_prop:s0 tclass=file permissive=0
03-15 11:46:13.395 W/Gralloc3(11599): mapper 3.x is not supported
03-15 11:46:13.398 E/libc    (11599): Access denied finding property "vendor.gralloc.disable_ahardware_buffer"
03-15 11:46:17.566 I/Util    (11599): runShellCommandGetOutput: Exited with code 0
03-15 11:46:20.423 W/yncthingandroi(11599): resources.arsc in APK '/data/app/cn.wps.moffice_eng-sX7LgN4rYlWIrTqwPLBJXw==/base.apk' is compressed.
03-15 11:46:20.569 W/yncthingandroi(11599): resources.arsc in APK '/data/app/com.facebook.katana-LdtwmG5zsIgzImZcUcoUzA==/base.apk' is compressed.
03-15 11:46:20.580 W/yncthingandroi(11599): resources.arsc in APK '/data/app/com.facebook.katana-LdtwmG5zsIgzImZcUcoUzA==/split_arservicesoptional.apk' is compressed.
03-15 11:46:20.581 W/yncthingandroi(11599): resources.arsc in APK '/data/app/com.facebook.katana-LdtwmG5zsIgzImZcUcoUzA==/split_groupsadminrulesedit.apk' is compressed.
03-15 11:46:20.582 W/yncthingandroi(11599): resources.arsc in APK '/data/app/com.facebook.katana-LdtwmG5zsIgzImZcUcoUzA==/split_rtc.apk' is compressed.
03-15 11:46:20.595 W/yncthingandroi(11599): resources.arsc in APK '/data/app/com.facebook.orca-14T_h7P-2yaCgko8ZNLHYw==/base.apk' is compressed.
03-15 11:46:20.798 W/yncthingandroi(11599): resources.arsc in APK '/data/app/com.instagram.android-n8jxzraZ-a44-LM6Cil1Bg==/base.apk' is compressed.
03-15 11:46:21.171 I/Choreographer(11599): Skipped 49 frames!  The application may be doing too much work on its main thread.
03-15 11:46:21.180 I/OpenGLRenderer(11599): Davey! duration=827ms; Flags=1, IntendedVsync=489556296837, Vsync=490372963471, OldestInputEvent=9223372036854775807, NewestInputEvent=0, HandleInputStart=490375887285, AnimationStart=490375904003, PerformTraversalsStart=490375951139, DrawStart=490382372337, SyncQueued=490382686504, SyncStart=490382714577, IssueDrawCommandsStart=490382814264, SwapBuffers=490383565410, FrameCompleted=490383946921, DequeueBufferDuration=75000, QueueBufferDuration=136000, 
03-15 11:46:21.334 W/sh      (19222): type=1400 audit(0.0:3057): avc: denied { read } for name="/" dev="dm-3" ino=2 scontext=u:r:untrusted_app_27:s0:c512,c768 tcontext=u:object_r:rootfs:s0 tclass=dir permissive=0
@Catfriend1
Copy link
Owner

Let's wait until Q is finished like I wrote on the other ticket in official... Maybe google fixes this before release.

@kvaster
Copy link
Author

kvaster commented Mar 15, 2019

Sounds like I've made a big mistake by updating to Android Q Beta :(
One of my main apps will not work...
Thanks for the info.

@Catfriend1
Copy link
Owner

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.

@kvaster
Copy link
Author

kvaster commented Mar 15, 2019

Topic question:
I've tried to investigate the problem. Is it worth to try to compile syncthing locally only for 32-bit ? Maybe it will work for now.

Offtopic:
I thought it's not yet ready for Android Q at least.

@Catfriend1
Copy link
Owner

It might also be a go or ndk issue...

@kvaster
Copy link
Author

kvaster commented Mar 15, 2019

A small, but good update (at least for me :) ... ):
I've started with this: syncthing/syncthing-android#1291
Following with this: golang/go#29674
And, in general, ending with this: shadowsocks/v2ray-plugin-android#6

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
For amr32 this should be 0x14c (change 0x04 to 0x20), but I've not checked.

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!

@Catfriend1
Copy link
Owner

How can that patch be applied automatically by a command line build script?!

@otbutz
Copy link

otbutz commented Mar 15, 2019

This is golang issue with bionic wrong tls

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.

@kvaster
Copy link
Author

kvaster commented Mar 16, 2019

@Catfriend1 , I will write python script today.

@kvaster
Copy link
Author

kvaster commented Mar 16, 2019

@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 :)

@Catfriend1
Copy link
Owner

Python is very good because that's the Lang of our build script

@NatoBoram
Copy link

@kvaster You might want to use code blocks to wrap your log output so you don't mention issues #1, #2, #3, #4 and #5 needlessly :)

@Catfriend1
Copy link
Owner

@kvaster What are the correct values for armeabi (arm32) and x86 so I could test on the emulator?

@Catfriend1
Copy link
Owner

What command line did you use with the readelf? Where to look?

@Catfriend1
Copy link
Owner

Do you mean:
readelf -l libsyncthing.so | grep "TLS"

@Catfriend1
Copy link
Owner

As I cant get anything to work, would you please upload your libsyncthing.so before and after the patching?

@Catfriend1
Copy link
Owner

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.
First way: - We find users having arm64-v8a or armeabi devices and test with them together
Second way: - We get x86 working with a patch and try it out on the emulator first.

@Catfriend1
Copy link
Owner

Tried to compile for x86 with -mstackrealign and -mno-stackrealign with no luck.

@Catfriend1
Copy link
Owner

Catfriend1 commented Mar 17, 2019

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 ```

@Catfriend1 Catfriend1 changed the title Crash on Android Q Beta Android Q Beta - Wrong Bionic TLS alignment (golang) Mar 17, 2019
@kvaster
Copy link
Author

kvaster commented Mar 17, 2019

Python script for patching alignment: https://pastebin.com/XZMKMmMd
Please note that this might not work for x86. Golang does not use TLS segment for arm/arm64, but I'm not sure if this is the same for x86. I've tested arm and arm64 patched libs on Pixel 3 XL with Android Q Beta.

@Catfriend1 Catfriend1 self-assigned this Mar 17, 2019
Catfriend1 added a commit that referenced this issue Mar 17, 2019
@Catfriend1
Copy link
Owner

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.

Invoking git fetch ...
Invoking git describe ...
Building for arm
Building syncthing version v1.1.0
artifact_patch_underaligned_tls: Patching underaligned TLS segment from 4 to 32
Finished build for arm
Building for arm64
Building syncthing version v1.1.0
artifact_patch_underaligned_tls: Patching underaligned TLS segment from 8 to 64
Finished build for arm64
Building for x86
Building syncthing version v1.1.0
Finished build for x86
All builds finished


BUILD SUCCESSFUL in 20s
1 actionable task: 1 executed

@Catfriend1 Catfriend1 added upstream testing Community test case - Notice: Consists of maybe unstable syncthing version or code and removed bug labels Mar 17, 2019
Catfriend1 pushed a commit that referenced this issue Mar 17, 2019
)

* Patch go wrong tls alignment for Android Q compatibility (fixes #370)

* Remove wrong x86 patch

* Incorporate @kvaster's Android Q patch into build script (fixes #370)

Thanks for the contributon!

* Update build script for Python 2 compatibility

* Second attempt for python 2
@Catfriend1
Copy link
Owner

@Catfriend1 Catfriend1 added this to the 1.1.1 milestone Mar 17, 2019
@Catfriend1
Copy link
Owner

golang/go#29249

@kvaster
Copy link
Author

kvaster commented Mar 17, 2019

@NatoBoram, I was using code blocks. Seems that it was due to missing emtpy line after tag. Not my fault.
@Catfriend1, thanks! Now I can use official build :)

@rprichard
Copy link

https://github.com/fuchsia-mirror/zircon/blob/0dbac4d37ca09eaebcb5d1337775b9dc78a4ac06/system/public/zircon/tls.h
https://android.googlesource.com/platform/bionic/+/master/docs/elf-tls.md#Thread_Specific-Memory-Layout
https://stackoverflow.com/questions/897105/c-alignment-at-not-a-power-of-2/897111#897111
android-ndk/ndk#635
@rprichard: Which are the correct values for patching x86 tls alignment successfully?

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 executable's TLS segment is underaligned: alignment is 8, needs to be at least 64 for ARM64 Bionic error. I think it works because Golang is not actually using the TLS segment.

@Catfriend1
Copy link
Owner

@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)

@Catfriend1
Copy link
Owner

@jaseemabid does syncthing fork work on your android q device?

@jaseemabid
Copy link

@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?

@Catfriend1
Copy link
Owner

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.

@CedricCabessa
Copy link

+1 your patch fixes the issue on my pixel2

syncthing/syncthing-android#1291 (comment)

@danblah
Copy link

danblah commented Mar 28, 2019

+1 your patch fixes the issue on my pixel3xl

syncthing/syncthing-android#1291 (comment)

@Catfriend1
Copy link
Owner

Does this apk also work on your q phones?
https://github.com/Catfriend1/syncthing-android/releases/download/v1.1.1.1/com.github.catfriend1.syncthingandroid_v1.1.1.1_4f461dce.apk

It's including an official patch on the go side instead of the binary quick win.

@CedricCabessa
Copy link

works for me @Catfriend1 🎉

@Catfriend1
Copy link
Owner

@danblah ?

@kbtombul
Copy link

kbtombul commented Apr 7, 2019

@Catfriend1 Works for me on Pixel 2 XL with Q Beta 2.

@bacomino
Copy link

Does this apk also work on your q phones?

Works like a charm, Thank you !

Oneplus 6 A6003 Android Q DP2

@Catfriend1
Copy link
Owner

Thank you very much for the provided feedback. :-)

Catfriend1 added a commit that referenced this issue Sep 3, 2019
- 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)
@Catfriend1
Copy link
Owner

@Athanasius well, here's a version to fill the time until release....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build testing Community test case - Notice: Consists of maybe unstable syncthing version or code upstream
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants