Investigating a crash at startup

[Phil Krylov]

Hello,

I have a program here (IBM Translation Manager v.6.0.4) which crashes
wine at startup. The program loads a DLL, and Wine starts importing
this DLL's imports from kernel32.dll. It crashes on the first import
in dlls/ntdll/loader.c:453, which says:

thunk_list->u1.Function = (PDWORD)find_named_export( imp_mod, exports, exp_size,
                                                     pe_name->Name, pe_name->Hint );

This causes an exception because thunk_list happens to be located in
.rdata section of the DLL, which is readonly.

Currently I have hacked this around by making all PE sections
read-write in map_image() in dlls/ntdll/virtual.c, and the program
works.

If a Wine guru would like to investigate this problem, I can give any
support (e.g., ssh access to my computer with the program).
If not - I will be using this hacked version of Wine.

[Filip Navara]

Hi!

In ReactOS we are unprotecting the IAT (Import Address Table) before 
every modification and restoring the previous access rights after all 
the modifications are done using NtProtectVirtualMemory. I'll try to 
write a patch for Wine tommorow if no one will do it earlier, but I 
can't test it...

Regards,
Filip

Phil Krylov wrote:
> Hello,
>
> I have a program here (IBM Translation Manager v.6.0.4) which crashes
> wine at startup. The program loads a DLL, and Wine starts importing
> this DLL's imports from kernel32.dll. It crashes on the first import
> in dlls/ntdll/loader.c:453, which says:
>
> thunk_list->u1.Function = (PDWORD)find_named_export( imp_mod, exports, exp_size,
>                                                     pe_name->Name, pe_name->Hint );
>
> This causes an exception because thunk_list happens to be located in
> .rdata section of the DLL, which is readonly.
>
> Currently I have hacked this around by making all PE sections
> read-write in map_image() in dlls/ntdll/virtual.c, and the program
> works.
>
> If a Wine guru would like to investigate this problem, I can give any
> support (e.g., ssh access to my computer with the program).
> If not - I will be using this hacked version of Wine.

[Filip Navara]

Hi!

One insteresting thing is that OpenOffice does the same and was reported 
to work under Wine. Anyway, an experimental patch is attached (it's 
completely untested).

Regards,
Filip

Phil Krylov wrote:
> Hello,
>
> I have a program here (IBM Translation Manager v.6.0.4) which crashes
> wine at startup. The program loads a DLL, and Wine starts importing
> this DLL's imports from kernel32.dll. It crashes on the first import
> in dlls/ntdll/loader.c:453, which says:
>
> thunk_list->u1.Function = (PDWORD)find_named_export( imp_mod, exports, exp_size,
>                                                     pe_name->Name, pe_name->Hint );
>
> This causes an exception because thunk_list happens to be located in
> .rdata section of the DLL, which is readonly.
>
> Currently I have hacked this around by making all PE sections
> read-write in map_image() in dlls/ntdll/virtual.c, and the program
> works.
>
> If a Wine guru would like to investigate this problem, I can give any
> support (e.g., ssh access to my computer with the program).
> If not - I will be using this hacked version of Wine.

--- dlls/ntdll/loader.c	Tue Apr 20 00:36:30 2004
+++ dlls/ntdll/loader.c	Sat Jun 12 16:23:34 2004
@@ -371,6 +371,9 @@
     WCHAR buffer[32];
     char *name = get_rva( module, descr->Name );
     DWORD len = strlen(name) + 1;
+    PVOID protect_base;
+    DWORD protect_size = 0;
+    DWORD protect_old;

thunk_list = get_rva( module, (DWORD)descr->FirstThunk );
     if (descr->u.OriginalFirstThunk)
@@ -403,6 +406,20 @@
         return NULL;
     }

+    /* unprotect the import address table since it can be located in
+     * readonly section */
+    while (import_list[protect_size].u1.Ordinal)
+        protect_size++;
+    protect_base = import_list;
+    protect_size *= sizeof(PVOID *);
+    status = NtProtectVirtualMemory( GetCurrentProcess(), &protect_base,
+                                     &protect_size, PAGE_READWRITE, &protect_old );
+    if (status)
+    {
+        ERR("Can't unprotect IAT for %s\n", name);
+        return NULL;
+    }
+
     imp_mod = wmImp->ldr.BaseAddress;
     exports = RtlImageDirectoryEntryToData( imp_mod, TRUE, IMAGE_DIRECTORY_ENTRY_EXPORT, &exp_size );

@@ -427,6 +444,11 @@
             import_list++;
             thunk_list++;
         }
+
+        /* restore old protection of the import address table */
+        NtProtectVirtualMemory( GetCurrentProcess(), &protect_base,
+                                &protect_size, protect_old, NULL );
+        
         return wmImp;
     }

@@ -463,6 +485,11 @@
         import_list++;
         thunk_list++;
     }
+
+    /* restore old protection of the import address table */
+    NtProtectVirtualMemory( GetCurrentProcess(), &protect_base,
+                            &protect_size, protect_old, NULL );
+    
     return wmImp;
 }

--- dlls/ntdll/virtual.c	Tue Apr  6 23:13:48 2004
+++ dlls/ntdll/virtual.c	Sat Jun 12 17:10:06 2004
@@ -511,6 +511,10 @@
                            int delta, DWORD total_size )
 {
     IMAGE_BASE_RELOCATION *rel;
+    NTSTATUS status;
+    PVOID protect_base;
+    DWORD protect_size = page_size;
+    DWORD protect_old;

TRACE_(module)( "relocating from %p-%p to %p-%p\n",
                     base - delta, base - delta + total_size, base, base + total_size );
@@ -537,6 +541,16 @@

TRACE_(module)("%ld relocations for page %lx\n", rel->SizeOfBlock, rel->VirtualAddress);

+        /* unprotect the page we're about to relocate */
+        protect_base = page;
+        status = NtProtectVirtualMemory( GetCurrentProcess(), &protect_base,
+                                         &protect_size, PAGE_READWRITE, &protect_old );
+        if (status)
+        {
+            ERR("Can't unprotect page during relocation\n");
+            return 0;
+        }
+
         /* patching in reverse order */
         for (i = 0 ; i < count; i++)
         {
@@ -561,6 +575,10 @@
                 break;
             }
         }
+
+        /* restore old protection */
+        NtProtectVirtualMemory( GetCurrentProcess(), &protect_base,
+                                &protect_size, protect_old, NULL );
     }
     return 1;
 }

[Filip Navara]

Now with the correct patch...

- Filip

Filip Navara wrote:
> Hi!
>
> One insteresting thing is that OpenOffice does the same and was 
> reported to work under Wine. Anyway, an experimental patch is attached 
> (it's completely untested).
>
> Regards,
> Filip
>
> Phil Krylov wrote:
> > Hello,
> >
> > I have a program here (IBM Translation Manager v.6.0.4) which crashes
> > wine at startup. The program loads a DLL, and Wine starts importing
> > this DLL's imports from kernel32.dll. It crashes on the first import
> > in dlls/ntdll/loader.c:453, which says:
> >
> > thunk_list->u1.Function = (PDWORD)find_named_export( imp_mod, 
> > exports, exp_size,
> >                                                     pe_name->Name, 
> > pe_name->Hint );
> >
> > This causes an exception because thunk_list happens to be located in
> > .rdata section of the DLL, which is readonly.
> >
> > Currently I have hacked this around by making all PE sections
> > read-write in map_image() in dlls/ntdll/virtual.c, and the program
> > works.
> >
> > If a Wine guru would like to investigate this problem, I can give any
> > support (e.g., ssh access to my computer with the program).
> > If not - I will be using this hacked version of Wine.

--- dlls/ntdll/loader.c	Tue Apr 20 00:36:30 2004
+++ dlls/ntdll/loader.c	Sat Jun 12 16:23:34 2004
@@ -371,6 +371,9 @@
     WCHAR buffer[32];
     char *name = get_rva( module, descr->Name );
     DWORD len = strlen(name) + 1;
+    PVOID protect_base;
+    DWORD protect_size = 0;
+    DWORD protect_old;

thunk_list = get_rva( module, (DWORD)descr->FirstThunk );
     if (descr->u.OriginalFirstThunk)
@@ -403,6 +406,20 @@
         return NULL;
     }

+    /* unprotect the import address table since it can be located in
+     * readonly section */
+    while (import_list[protect_size].u1.Ordinal)
+        protect_size++;
+    protect_base = thunk_list;
+    protect_size *= sizeof(PVOID *);
+    status = NtProtectVirtualMemory( GetCurrentProcess(), &protect_base,
+                                     &protect_size, PAGE_READWRITE, &protect_old );
+    if (status)
+    {
+        ERR("Can't unprotect IAT for %s\n", name);
+        return NULL;
+    }
+
     imp_mod = wmImp->ldr.BaseAddress;
     exports = RtlImageDirectoryEntryToData( imp_mod, TRUE, IMAGE_DIRECTORY_ENTRY_EXPORT, &exp_size );

@@ -427,6 +444,11 @@
             import_list++;
             thunk_list++;
         }
+
+        /* restore old protection of the import address table */
+        NtProtectVirtualMemory( GetCurrentProcess(), &protect_base,
+                                &protect_size, protect_old, NULL );
+        
         return wmImp;
     }

@@ -463,6 +485,11 @@
         import_list++;
         thunk_list++;
     }
+
+    /* restore old protection of the import address table */
+    NtProtectVirtualMemory( GetCurrentProcess(), &protect_base,
+                            &protect_size, protect_old, NULL );
+    
     return wmImp;
 }

--- dlls/ntdll/virtual.c	Tue Apr  6 23:13:48 2004
+++ dlls/ntdll/virtual.c	Sat Jun 12 17:10:06 2004
@@ -511,6 +511,10 @@
                            int delta, DWORD total_size )
 {
     IMAGE_BASE_RELOCATION *rel;
+    NTSTATUS status;
+    PVOID protect_base;
+    DWORD protect_size = page_size;
+    DWORD protect_old;

TRACE_(module)( "relocating from %p-%p to %p-%p\n",
                     base - delta, base - delta + total_size, base, base + total_size );
@@ -537,6 +541,16 @@

TRACE_(module)("%ld relocations for page %lx\n", rel->SizeOfBlock, rel->VirtualAddress);

+        /* unprotect the page we're about to relocate */
+        protect_base = page;
+        status = NtProtectVirtualMemory( GetCurrentProcess(), &protect_base,
+                                         &protect_size, PAGE_READWRITE, &protect_old );
+        if (status)
+        {
+            ERR("Can't unprotect page during relocation\n");
+            return 0;
+        }
+
         /* patching in reverse order */
         for (i = 0 ; i < count; i++)
         {
@@ -561,6 +575,10 @@
                 break;
             }
         }
+
+        /* restore old protection */
+        NtProtectVirtualMemory( GetCurrentProcess(), &protect_base,
+                                &protect_size, protect_old, NULL );
     }
     return 1;
 }

Re[2]: Investigating a crash at startup

[Phil Krylov]

Hello Filip,

Monday, June 14, 2004, 12:26:18 AM, you wrote:

FN> Now with the correct patch...

Thanks Filip, it works fine here. Any chance that it will be committed
to CVS?