[Bug 12332] Microsoft Windows Server 2003 DDK SP1 installer crashes ('setupapi.SetupCloseFileQueue' should do proper handle validation before accessing members)

https://bugs.winehq.org/show_bug.cgi?id=12332

Anastasius Focht  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]
            Summary|driver dev kit won't        |Microsoft Windows Server
                   |install                     |2003 DDK SP1 installer
                   |                            |crashes
                   |                            |('setupapi.SetupCloseFileQu
                   |                            |eue' should do proper
                   |                            |handle validation before
                   |                            |accessing members)
           Severity|enhancement                 |normal

--- Comment #11 from Anastasius Focht  ---
Hello folks,

confirming.

Jeff (comment #9) already correctly analysed the issue - 'SetupCloseFileQueue'
gets the wrong handle.

It seems the handle is actually the context returned from
'SetupInitDefaultQueueCallbackEx'.

Relevant part of trace log:

--- snip ---
$ WINEDEBUG=+tid,+seh,+relay,+setupapi wine ./Kitsetup.exe >>~/log.txt 2>&1
...
002c:Call setupapi.SetupOpenFileQueue() ret=010088b6
...
002c:Ret  setupapi.SetupOpenFileQueue() retval=00171708 ret=010088b6
002c:Call
setupapi.SetupInitDefaultQueueCallbackEx(000100d4,000100d4,00000401,00000000,00000000)
ret=010088f7
...
002c:Ret  setupapi.SetupInitDefaultQueueCallbackEx() retval=006a1450
ret=010088f7
002c:Call
setupapi.SetupInstallFilesFromInfSectionA(00662c90,00000000,00171708,01002044
"DefaultInstall",00676af8 "C:\WINDDK\3790.1830",00000004) ret=0100892d 
...
002c:Ret  setupapi.SetupInstallFilesFromInfSectionA() retval=00000001
ret=0100892d
002c:Call setupapi.SetupCommitFileQueueA(00000000,00171708,01007a1d,006a1450)
ret=0100895f
002c:Call
setupapi.SetupDefaultQueueCallbackA(006a1450,00000001,00000000,00000000)
ret=01007bbc
002c:trace:setupapi:SetupDefaultQueueCallbackA start queue
002c:Ret  setupapi.SetupDefaultQueueCallbackA() retval=00000001 ret=01007bbc
002c:Call
setupapi.SetupDefaultQueueCallbackA(006a1450,00000003,00000000,00000106)
ret=01007bbc
002c:trace:setupapi:SetupDefaultQueueCallbackA start subqueue 0 count 262
002c:Ret  setupapi.SetupDefaultQueueCallbackA() retval=00000001 ret=01007bbc 
...
002c:Ret  setupapi.SetupCommitFileQueueA() retval=00000001 ret=0100895f
002c:Call setupapi.SetupInstallFromInfSectionA(00000000,00662c90,01002044
"DefaultInstall",00000004,00000000,00b5e4b8
"Z:\home\focht\iso\Common",00000000,00000000,00000000,00000000,00000000)
ret=010089a5 
...
002c:Ret  setupapi.SetupInstallFromInfSectionA() retval=00000001 ret=010089a5
002c:Call setupapi.SetupCloseFileQueue(006a1450) ret=01008a50
002c:trace:seh:raise_exception code=c0000005 flags=0 addr=0x7e536585
ip=7e536585 tid=002c
002c:trace:seh:raise_exception  info[0]=00000000
002c:trace:seh:raise_exception  info[1]=4351505b
002c:trace:seh:raise_exception  eax=43515053 ebx=00000000 ecx=00b5db20
edx=00000004 esi=00b5db58 edi=00b5db24
002c:trace:seh:raise_exception  ebp=00b5dad8 esp=00b5da90 cs=0023 ds=002b
es=002b fs=0063 gs=006b flags=00010206
002c:trace:seh:call_stack_handlers calling handler at 0x7bcb50ab code=c0000005
flags=0
002c:Call KERNEL32.UnhandledExceptionFilter(00b5d574) ret=7bcb50e5
wine: Unhandled page fault on read access to 0x4351505b at address 0x7e536585
(thread 002c), starting debugger...
...
--- snip ---

SetupOpenFileQueue -> 00171708
SetupInitDefaultQueueCallbackEx -> 006a1450
SetupCloseFileQueue -> 006a1450

Seems a stupid mistake on behalf of Microsoft.
There is no other code path in the installer that can produce a different
result.

It's not uncommon that internal data structures which are opaque to the outside
(handle), contain some "magic id" member to uniquely identify/distinguish them
from other structures.

https://source.winehq.org/git/wine.git/blob/dcab5fe61bed87b291901ceff686894a...

--- snip ---
  39 /* context structure for the default queue callback */
  40 struct default_callback_context
  41 {
  42     DWORD     magic;
  43     HWND      owner;
  44     DWORD     unk1[4];
  45     DWORD_PTR unk2[7];
  46     HWND      progress;
  47     UINT      message;
  48     DWORD_PTR unk3[5];
  49 };
...
1484 PVOID WINAPI SetupInitDefaultQueueCallbackEx( HWND owner, HWND progress,
UINT msg,
1485                                               DWORD reserved1, PVOID
reserved2 )
1486 {
1487     struct default_callback_context *context;
1488 
1489     if ((context = HeapAlloc( GetProcessHeap(), HEAP_ZERO_MEMORY,
sizeof(*context) )))
1490     {
1491         context->magic    = 0x43515053; /* "SPQC" */
1492         context->owner    = owner;
1493         context->progress = progress;
1494         context->message  = msg;
1495     }
1496     return context;
1497 }
--- snip ---

Why not doing the same for 'struct file_queue' to detect such mishap?

https://source.winehq.org/git/wine.git/blob/dcab5fe61bed87b291901ceff686894a...

--- snip ---
  71 struct file_queue
  72 {
  73     struct file_op_queue copy_queue;
  74     struct file_op_queue delete_queue;
  75     struct file_op_queue rename_queue;
  76     DWORD                flags;
  77 };

--- snip ---

If the passed handle is detected as invalid (magic not found) it should error
out without further operation.

$ sha1sum 1830_usa_ddk.iso 
0d2154d88a5ee252cc908630c77863bb42777387  1830_usa_ddk.iso

$ du -sh 1830_usa_ddk.iso 
231M    1830_usa_ddk.iso

$ wine --version
wine-1.8-rc2

Regards