[PATCH] ntdll: Improve handling of THUMB_MOV32 relocations

Martin Storsjö martin at martin.st
Thu Jan 2 14:57:28 CST 2014


On Thu, 2 Jan 2014, André Hentschel wrote:

> Am 02.01.2014 17:01, schrieb Martin Storsjo:
>> Since the delta variable actually is signed, the delta > 0xffff
>> condition could be false even if the high part of the delta
>> was nonzero, ending up with not updating the high half of the
>> relocation at all.
>>
>> Additionally, carry any overflow from the low word into the
>> high word delta.
>
>
> Hi,
> Do you have an App that triggers that?

I've got a 5 MB .exe that triggers this occasionally. If I disable ASLR in 
the kernel, it doesn't trigger. I guess the bigger the image, the larger 
the risk that ASLR will force the exe to be relocated, triggering the 
issue.

Conversely, if I change map_image to not try to load the image at the base 
address (as if it would have failed to load at that address due to ASLR), 
this triggers every time even with a small hello world .exe.

> So you mean the case when the sign bit is used, e.g. 10101010b... Then the delta is negative, but is everything set correctly in the instruction?

Yes, the calculation seem to work just fine with my patch applied. With 
the 5 MB .exe, the relocation that is done on startup was:

trace:module:map_image relocating from 0x400000-0xe6e000 to 0xb58a0000-0xb630e000

So the delta really isn't negative per se, but it's too large to represent 
with a 32 bit signed integer.


So to reproduce easily, apply this diff to your tree, then try to execute 
http://albin.abo.fi/~mstorsjo/helloworld-arm.exe (a plain hello world exe 
compiled with msvc 2012 for winrt). At least here, it tries to relocate 
the .exe from 0x400000-0x410000 to 0xb6200000-0xb6210000.

index e93ce2e..a6ffe38 100644
--- a/dlls/ntdll/virtual.c
+++ b/dlls/ntdll/virtual.c
@@ -1077,9 +1077,6 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, 
char *base, SIZE_T total_siz

      server_enter_uninterrupted_section( &csVirtual, &sigset );

-    if (base >= (char *)address_space_start)  /* make sure the DOS area 
remains free */
-        status = map_view( &view, base, total_size, mask, FALSE,
-                           VPROT_COMMITTED | VPROT_READ | VPROT_EXEC | 
VPROT_WRITECOPY | VPROT_IMAGE );

      if (status != STATUS_SUCCESS)
          status = map_view( &view, NULL, total_size, mask, FALSE,



>> ---
>>  dlls/ntdll/loader.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c
>> index e0907e7..1f13424 100644
>> --- a/dlls/ntdll/loader.c
>> +++ b/dlls/ntdll/loader.c
>> @@ -2230,19 +2230,19 @@ IMAGE_BASE_RELOCATION * WINAPI LdrProcessRelocationBlock( void *page, UINT count
>>              DWORD inst = *(INT_PTR *)((char *)page + offset);
>>              DWORD imm16 = ((inst << 1) & 0x0800) + ((inst << 12) & 0xf000) +
>>                            ((inst >> 20) & 0x0700) + ((inst >> 16) & 0x00ff);
>> +            int hi_delta;
>
> Why you're using int here? DWORD looks much better for this.

No particular reason, DWORD should work just fine as well, it seems. I can 
send a new patch with that changed if you're ok with the rest of it - just 
let me know.

// Martin


More information about the wine-devel mailing list