Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash on start #2363

Open
AlxVD opened this issue Jan 4, 2025 · 7 comments
Open

Crash on start #2363

AlxVD opened this issue Jan 4, 2025 · 7 comments

Comments

@AlxVD
Copy link

AlxVD commented Jan 4, 2025

Brief description of your issue

SI crashes right after showing main window
SystemInformer_DumpFile_VEWCYOHPCXDRMJS.zip

Steps to reproduce (optional)

No response

Expected behavior (optional)

No response

Actual behavior (optional)

No response

Environment (optional)

3.2.25004.614
Win10 22h2 (19045.5247)
@ge0rdi
Copy link
Contributor

ge0rdi commented Jan 4, 2025

Crash happened when accessing mapped memory during image coherency check:

ExceptionAddress: 00007ff754086400 (SystemInformer!PhImageCoherencyRelocationCallback+0x0000000000000040)
   ExceptionCode: c0000005 (Access violation)
  ExceptionFlags: 00000000
NumberParameters: 2
   Parameter[0]: 0000000000000000
   Parameter[1]: 0000025af32b1fc8
Attempt to read from address 0000025af32b1fc8

Though the accessed memory seems to be valid:

Usage:                  <unknown>
Base Address:           0000025a`f3270000
End Address:            0000025a`f33bd000
Region Size:            00000000`0014d000 (   1.301 MB)
State:                  00001000          MEM_COMMIT
Protect:                00000002          PAGE_READONLY
Type:                   00040000          MEM_MAPPED
Allocation Base:        0000025a`f3270000
Allocation Protect:     00000002          PAGE_READONLY

So that's rather weird (maybe some kinf of race?).

Here is stack trace for reference:

 # Child-SP          RetAddr               Call Site
00 000000df`f2fff820 00007ff7`54055a95     SystemInformer!PhImageCoherencyRelocationCallback+0x40 [phlib\imgcoherency.c @ 170] 
01 000000df`f2fff890 00007ff7`540865ee     SystemInformer!PhMappedImageEnumerateRelocations+0x115 [phlib\mapimg.c @ 4233] 
02 000000df`f2fff8e0 00007ff7`540879be     SystemInformer!PhpCreateImageCoherencyContext+0x14e [phlib\imgcoherency.c @ 265] 
03 000000df`f2fff990 00007ff7`54087c45     SystemInformer!PhpGetModuleCoherency+0x21e [phlib\imgcoherency.c @ 1301] 
04 000000df`f2fffa40 00007ff7`53f6bc1a     SystemInformer!PhGetProcessImageCoherency+0x1c5 [phlib\imgcoherency.c @ 1390] 
05 000000df`f2fffb30 00007ff7`53f6bd5a     SystemInformer!PhpProcessQueryStage2+0x29a [SystemInformer\procprv.c @ 1027] 
06 000000df`f2fffc20 00007ff7`5404de86     SystemInformer!PhpProcessQueryStage2Worker+0x5a [SystemInformer\procprv.c @ 1073] 
07 (Inline Function) --------`--------     SystemInformer!PhpExecuteWorkQueueItem+0x11 [phlib\workqueue.c @ 321] 
08 000000df`f2fffc50 00007ff7`53ffa735     SystemInformer!PhpWorkQueueThreadStart+0x3e6 [phlib\workqueue.c @ 460] 
09 000000df`f2fffee0 00007ff9`0cd07374     SystemInformer!PhpBaseThreadStart+0x75 [phlib\basesup.c @ 195] 
0a 000000df`f2ffff20 00007ff9`0d67cc91     kernel32!BaseThreadInitThunk+0x14
0b 000000df`f2ffff50 00000000`00000000     ntdll!RtlUserThreadStart+0x21

@AlxVD
Copy link
Author

AlxVD commented Jan 4, 2025

I can confirm that turning off "check images for coherency" allows SI to run.

@jxy-s jxy-s added bug and removed needs-triage labels Jan 4, 2025
@jxy-s
Copy link
Member

jxy-s commented Jan 4, 2025

@AlxVD could you attach here or email me a normal or complete dump when this issue reproduces? With a complete dump I can identify what file is associated with the crash, it might be a malformed PE file.

The relocationDirectory and SizeOfBlock should probably be probed inside of PhMappedImageEnumerateRelocations. Although it appears from @ge0rdi's analysis that the address is valid. I'm not sure if is actually within range of the mapping.

Additionally, our probing logic around mapped files should be using PhProbeForRead instead of PhProbeAddress to ensure that the mapping can be paged in - else it will crash with STATUS_IN_PAGE_ERROR, but that's not what we see in this case.

@AlxVD
Copy link
Author

AlxVD commented Jan 4, 2025

This is a "normal" dump but I'll make "full", ok.

@AlxVD
Copy link
Author

AlxVD commented Jan 4, 2025

Just sent to @live.com

@jxy-s
Copy link
Member

jxy-s commented Jan 5, 2025

There is a relocation block for FaceLift_x64.exe that is zero:

0:011> dx -r1 relocationDirectory
relocationDirectory                 : 0x25c6fd62400 [Type: _IMAGE_BASE_RELOCATION *]
    [+0x000] VirtualAddress   : 0x0 [Type: unsigned long]
    [+0x004] SizeOfBlock      : 0x0 [Type: unsigned long]

This results in the following logic underflowing:

        relocationCount = (relocationDirectory->SizeOfBlock - sizeof(IMAGE_BASE_RELOCATION)) / sizeof(IMAGE_RELOCATION_RECORD);
        relocations = PTR_ADD_OFFSET(relocationDirectory, RTL_SIZEOF_THROUGH_FIELD(IMAGE_BASE_RELOCATION, SizeOfBlock));

When the callback is invoked it enumerates past the end of the mapping:

000000d0`466ff160 unsigned long RelocationCount = 0xfffffffc

0:011> dx -r1 ((SystemInformer!_PH_MAPPED_IMAGE *)0x25c6c3fd148)
((SystemInformer!_PH_MAPPED_IMAGE *)0x25c6c3fd148)                 : 0x25c6c3fd148 [Type: _PH_MAPPED_IMAGE *]
    [+0x000] Signature        : 0x5a4d [Type: unsigned short]
    [+0x008] ViewBase         : 0x25c6fb00000 [Type: void *]
    [+0x010] ViewSize         : 0x26f200 [Type: unsigned __int64]
    [+0x018] NtHeaders32      : 0x25c6fb00100 [Type: _IMAGE_NT_HEADERS *]
    [+0x018] NtHeaders64      : 0x25c6fb00100 [Type: _IMAGE_NT_HEADERS64 *]
    [+0x018] NtHeaders        : 0x25c6fb00100 [Type: _IMAGE_NT_HEADERS64 *]
    [+0x020] NumberOfSections : 0x6 [Type: unsigned long]
    [+0x028] Sections         : 0x25c6fb00208 [Type: _IMAGE_SECTION_HEADER *]
    [+0x030] Magic            : 0x20b [Type: unsigned short]
    [+0x018] Header           : 0x25c6fb00100 [Type: _ELF_IMAGE_HEADER *]
    [+0x020] Headers32        : 0x6 [Type: _ELF_IMAGE_HEADER32 *]
    [+0x020] Headers64        : 0x6 [Type: _ELF_IMAGE_HEADER64 *]

0:011> r
Last set context:
rax=0000000000000000 rbx=0000025c6fd70000 rcx=0000025c6cb488b0
rdx=0000000000000000 rsi=0000025c6fd62400 rdi=00000000ffff9200
rip=00007ff754086400 rsp=000000d0466ff0d0 rbp=0000025c6c3fd120
 r8=00000000000002c3  r9=0000025c6cad1570 r10=00000ffeea800012
r11=0200200220140002 r12=0000025c6c3fd120 r13=0000025c6cad3434
r14=0000025c6c3fd148 r15=0000025c6fd67da0
iopl=0         nv up ei pl nz na po nc
cs=0033  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00010206
SystemInformer!PhImageCoherencyRelocationCallback+0x40:
00007ff7`54086400 0fb713          movzx   edx,word ptr [rbx] ds:0000025c`6fd70000=????

This regressed due to the following commit: 1cd217d#diff-385ab4b333bba97f7c119a641957c4bcef049796dd3eca653ae005e450d14b01L220

The implementation of PhGetMappedImageRelocations correctly handles this condition. But the new implementation (PhMappedImageEnumerateRelocations) neglected to include it.

I will have a patch for this shortly. It will be in the next build 👍.

@jxy-s jxy-s self-assigned this Jan 5, 2025
@jxy-s
Copy link
Member

jxy-s commented Jan 5, 2025

Fixed here: 304b7db

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants