Bug#908412: Followup

[Jiri Palecek <jpalecek@web.de>]

Hello,

I think this bug is caused by the addition of the Diablo patch. 
Certainly, removing it fixed it.

--- a/dlls/ntdll/loader.c
+++ b/dlls/ntdll/loader.c
@@ -870,6 +870,9 @@ static SHORT alloc_tls_slot( LDR_MODULE
     size = dir->EndAddressOfRawData - dir->StartAddressOfRawData;
     if (!size && !dir->SizeOfZeroFill && !dir->AddressOfCallBacks) 
return -1;

+    if (!tls_dirs)
+        return -1;
+
     for (i = 0; i < tls_module_count; i++)
     {
         if (!tls_dirs[i].StartAddressOfRawData && 
!tls_dirs[i].EndAddressOfRawData &&

I think the patch is simply wrong. It doesn't really fix any obvious bug 
in the logic and certainly shouldn't "fix out-of-bounds read when 
tls_dirs is empty" as the code already doesn't access tls_dirs when it 
is empty, bc. tls_module_count should be zero then. The code that 
follows clearly handles tls_dirs being null by allocating some memory 
and setting tls_dirs.

If anything, the patch fixes Diablo by obstructing normal function of 
the loader. I don't think that is a wise move to make.

Do you have any indication of an actual out-of-bounds read happening 
there when running Diablo? Or what exactly were you thinking when 
writing the patch? The messages on the board ( 
https://us.battle.net/forums/en/bnet/topic/20760475943?page=4) indicate 
some Windows users also see crashes.

I suggest the patch should be abandoned.

Regards

    Jiri Palecek