msys2 / MINGW-packages

Package scripts for MinGW-w64 targets to build under MSYS2.
https://packages.msys2.org
BSD 3-Clause "New" or "Revised" License
2.29k stars 1.22k forks source link

ObjC in both clang and gcc broken without -Wl,--default-image-base-low #8139

Open Midar opened 3 years ago

Midar commented 3 years ago

It seems that with the new binutils, ObjC code is broken on Win64, no matter whether using clang or GCC. Adding -Wl,--default-image-base-low fixes it. Looking through it with the debugger, it seems to just jump into a random, non-mapped address after running a bunch of ctors. I suspect a relocation is missing for some entry in the ctors array?

I'll try win32 next.

revelator commented 3 years ago

still a few quirks with the new hardening, but thats why we need feedback if anything is not reacting as it should :)

Midar commented 3 years ago

Happy to help debugging it, provide a reproducer, etc.

// Edit: Oh, and win32 did not have the same issue.

revelator commented 3 years ago

aye the high image base is 64 bit only so :)

revelator commented 3 years ago

the --image-base-low/high flags where added to get around cases where the default high image address would break something. it is not part of of the original binutils source code and we actually had to add a flag to ignore it in clang since clang does not understand it.

Midar commented 3 years ago

Ah, that explains why my cross-compiling toolchain does not accept it at all :).

For now, I added a workaround: https://objfw.nil.im/file?name=configure.ac&ci=tip&ln=352,369

I am assuming the side effect is reduced randomness for ASLR?

revelator commented 3 years ago

more a security feature of 64 bit making it harder to brute force attack a program built with it on, there is a good explanation here https://www.fireeye.com/blog/threat-research/2020/03/six-facts-about-address-space-layout-randomization-on-windows.html

some programs cannot by default use aslr though so each bug report add's to where we need to research other measures. in most cases the programs that cannot use aslr are targeted for earlier OS like WinXP though but there are a few edge cases like code that uses shared memory which explodes rather spectacularily if aslr is on because it relies on a constant base adress. The cygwin and msys2 dll are one such example.

jeremyd2019 commented 3 years ago

I'd like to see a reproducer, preferably with instructions for somebody who has no experience building objective C. Based on the description, I expect it's what Microsoft called "latent pointer truncation" in their docs - something is probably truncating a pointer to 32 bits. Setting the ImageBase below 4GB causes Windows to restrict ASLR such that it won't use addresses that won't fit in 32 bits for the module (so yes, reduces the randomness)

revelator commented 3 years ago

in that case indeed :)

revelator commented 3 years ago

can latent pointer truncation be detected codevise ? might be a nifty feature for binutils :)

Midar commented 3 years ago

As for reproducing, the easiest way is to just

git clone https://github.com/ObjFW/ObjFW
cd ObjFW
./autogen.sh
./configure
vi buildsys.mk  # Remove default-image-base-low again
make WRAPPER=gdb

Compiles what I tested and immediately throws you into gdb in it.

revelator commented 3 years ago

most cases are botched attempts at porting old code to 64 bit we sometime end up with casts from 64 bit integers to 32 bit and then things like this happens... as for detecting latent pointer truncation it seems there is indeed a way but im not sure how to implement it since it depends on using VirtualAlloc with a very high stack base.

mati865 commented 3 years ago

I think it might just missing dllimport/dllexport.

Midar commented 3 years ago

dllimport/dllexport on a ctor?! How would that work? I mean, the ctors have to be in the same compilation unit anyway?

mati865 commented 3 years ago

Ah, it's about ctors. Completely missed that part when re-reading the issue few days later.

I suspect a relocation is missing for some entry in the ctors array?

Or relocations do have 32-bit offsets.

To rule out linking issues could you try those 2 linker flags separately?

Midar commented 3 years ago

--disable-auto-import is giving me plenty of undefined references. Some in my custom assembly (disabling that makes those go away, interestingly?), the others in compiler-generated code (mostly class references). Using only --disable-runtime-pseudo-reloc without --default-image-base-low links but crashes on run.

jeremyd2019 commented 3 years ago

I'm glad to see you don't require runtime pseudo-relocs.

Auto import could be an issue. Especially where assembly is involved. I haven't gotten a chance to take a look yet, but maybe if I describe the potential pitfalls it will ring a bell with you. I apologize if you already know how DLL linkage works.

If you have a dllimport attribute on a function declaration for function foo, instead of generating the usual

call foo

the compiler will generate

call QWORD PTR[__imp_foo]

With auto-import, if the linker sees a call foo with no symbol foo, it looks for a symbol __imp_foo, and if that is found, it kind of abuses the Win32 dynamic loader to write a relocation at the immediate argument of the call instruction. On 32-bit this is fine, but on x64, the call instruction doesn't take a full 64-bit immediate, but rather a 32-bit relative immediate. If the call instruction is within +/- 2GB of the destination, this can work out, but once you get beyond that (due to ASLR using some of those more-significant bits) it just silently truncates, and you wind up jumping into no-mans land.

However, the import libraries created at least by BFD (and probably other linkers that use the .a format for Windows) generate both the __imp_foo symbol AND a foo thunk function which consists of

jmp QWORD PTR[__imp_foo]

So, if you link against a proper import library, symbol foo IS declared, and you avoid the auto-import relocation hack. But, BFD only generates those thunks for dllexported symbols that it knows are code, not data. It defaults to thinking any symbol is data, unless it includes the incantation

.def foo
.scl 2
.type 32
.endef

The compiler generates this in the ASM of functions it generates, but if you write pure-assembly functions you may need to include that on Windows targets.

jeremyd2019 commented 3 years ago

Ok, got a debug gcc and did some stepping. I'm seeing a bunk call at https://github.com/ObjFW/ObjFW/blob/master/tests/TestsAppDelegate.m#L197 It appears to be the call to TestsAppDelegate::alloc. More digging shows that objc_msg_lookup is being auto-imported. I confirmed that there was a proper thunk for it in libobjc.dll.a, so I tried explicitly linking the tests.exe with -lobjc. That solved the crash, but then I got a message that objc runtime: cannot find class OFObject. Rebuilding everything with -lobjc added to LDFLAGS results in a message that Module (null) version 8 doesn't match runtime 8.

Any of this make any sense from an ObjC standpoint?

jeremyd2019 commented 3 years ago

D'oh. This project provides a replacement for -lobjc and isn't trying to use it. I see that objc_msg_lookup is supplied in a .S file. So I think my mention of the .type thing is relevant here.

jeremyd2019 commented 3 years ago

The following patch moved it along a little further, but hit another after [Runtime] Calling a non-existent method via super: testing...

diff --git a/src/runtime/lookup-asm/lookup-asm-x86_64-win64.S b/src/runtime/lookup-asm/lookup-asm-x86_64-win64.S
index 381600fb..df22aba6 100644
--- a/src/runtime/lookup-asm/lookup-asm-x86_64-win64.S
+++ b/src/runtime/lookup-asm/lookup-asm-x86_64-win64.S
@@ -22,6 +22,10 @@

 .section .text
 .macro generate_lookup name not_found
+.def \name
+.scl 2
+.type 32
+.endef
 \name:
        testq   %rcx, %rcx
        jz      ret_nil
Midar commented 3 years ago

Yep, -lobjc isn't needed :). I also tried without the .S file (just remove the import in the lookup-asm.S file and remove the #ifndef OF_ASM_LOOKUP in lookup.m to enable the fallback) and it will still fail the same way with an immediate crash. (I should probably add a configure flag for this)

jeremyd2019 commented 3 years ago

Huh, seems to work for me after

diff --git a/src/runtime/lookup-asm/lookup-asm-x86_64-win64.S b/src/runtime/lookup-asm/lookup-asm-x86_64-win64.S
index 381600fb..0077ed09 100644
--- a/src/runtime/lookup-asm/lookup-asm-x86_64-win64.S
+++ b/src/runtime/lookup-asm/lookup-asm-x86_64-win64.S
@@ -22,6 +22,10 @@

 .section .text
 .macro generate_lookup name not_found
+.def \name
+.scl 2
+.type 32
+.endef
 \name:
        testq   %rcx, %rcx
        jz      ret_nil
@@ -70,6 +74,10 @@
 .endm

 .macro generate_lookup_super name lookup
+.def \name
+.scl 2
+.type 32
+.endef
 \name:
        movq    %rcx, %r8
        movq    (%rcx), %rcx
Midar commented 3 years ago

Did you remove --default-image-base-low and link everything without it? Because with --default-image-base-low it works ;)

jeremyd2019 commented 3 years ago

Yes 😄

Midar commented 3 years ago

Indeed, after that change it no longer crashes. That is weird, given that even with the C version it didn't work (undefined references). I might have just missed that the undefined references changes since there were so many :grinning:. However, utils/tests still don't link untiil I remove -Wl,--disable-runtime-pseudo-reloc,--disable-auto-import, but once I do, all tests run.

jeremyd2019 commented 3 years ago

I don't know why the class name symbols are not being properly imported (do objc classes take dllexport/dllimport decoration?)

Midar commented 3 years ago

They don't take anything - they are emitted by the compiler directly.

jeremyd2019 commented 3 years ago

I found https://github.com/llvm/llvm-project/commit/511f2e5a8955c90d6b9c176c31af2ec3add395fa though I don't know if that does anything useful under the hood.

But, I think auto import would be ok for those, other than not being able to use --disable-auto-import to catch more problematic things.

It looks like the .def/.endef cruft should be added to 32-bit windows as well, and to the of_forward{,_stret} functions. It also looks like of_forward calls imported functions from objfwrt, but that should be OK as it was linking to the thunks provided in the import library. Unless you want to change them to callq *(__imp_whatever) (or however that looks in AT&T syntax) to avoid the extra indirect jmp. Except that then won't work if statically linked, so may be better to leave it using the thunks.

lazka commented 3 years ago

It this still an issue? I see https://github.com/ObjFW/ObjFW/commit/d5b767bf3b470bc75c04519d65dc0c665ebf9bb8 which indicates it might be fixed.

Midar commented 3 years ago

Ah, sorry for forgetting to update the bug! Yes, this indeed works now, but there remains a problem with exception unwinding that probably is unrelated (but not sure) that for the heck of it I can't figure out (I'm getting an unexpected call to _Unwind_DeleteException resulting in double free). That issue happens on no other OS than Windows with MINGW :/.