Skip to content

Fix AV in FindRealCode and increment version number#8

Merged
dcristoloveanu merged 3 commits into
masterfrom
fix_AV_in_FindRealCode
Mar 11, 2021
Merged

Fix AV in FindRealCode and increment version number#8
dcristoloveanu merged 3 commits into
masterfrom
fix_AV_in_FindRealCode

Conversation

@dcristoloveanu
Copy link
Copy Markdown
Member

The issue here was that FindRealCode attempts to skip over the jmp mnemonics (most likely causing issues if detouring APIs).
When reading the addresses (absolute or relative) in the code of the exporting/importing DLLs, the code would not change the protect status to make sure it can read it. In some obscure and infrequent cases the page would have PAGE_NOACCESS and then a read would cause an AV.

Comment thread setup/vld-setup.iss Outdated
#define MyAppURL "http://vld.codeplex.com/"
#define MyAppRegKey "Software\Visual Leak Detector"
#define ConfigType "Release"
#define ConfigType "Debug"
Copy link
Copy Markdown

@mattdurak mattdurak Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug [](start = 20, length = 5)

Probably should leave this checked in as Release by default. #Resolved

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, changed it back to Release


In reply to: 591982836 [](ancestors = 591982836)

Comment thread src/utility.cpp Outdated
{
// we need to make sure we can read the first 3 ULONG_PTRs
DWORD old_protect;
// make sure we can read the first 3 pointers
Copy link
Copy Markdown

@mattdurak mattdurak Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure we can read the first 3 pointers [](start = 11, length = 42)

nit: says the same thing same as last comment #Resolved

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed


In reply to: 591983102 [](ancestors = 591983102)

Comment thread src/utility.cpp Outdated

// now that we got the offset, make sure we can read the code at the offset
DWORD old_protect_2;
if (VirtualProtect(pCode, sizeof(LPVOID), PAGE_EXECUTE_READ, &old_protect_2))
Copy link
Copy Markdown

@mattdurak mattdurak Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pCode [](start = 35, length = 5)

pNextInst + offset, here and below (or throw it in a variable for better readability) #Resolved

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


In reply to: 591983750 [](ancestors = 591983750)

Comment thread src/utility.cpp Outdated
else if (*(BYTE*)pCode == 0xE9) // JMP rel32
{
// Relative next instruction
PBYTE pNextInst = (PBYTE)((ULONG_PTR)pCode + 5);
Copy link
Copy Markdown

@mattdurak mattdurak Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[](start = 21, length = 1)

nit: random tabs detected (while you are in here making these changes) #Resolved

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


In reply to: 591984604 [](ancestors = 591984604)

Comment thread src/utility.cpp Outdated
#endif
#endif

// have a stack local array of the addresses, don;t want to use malloc for this
Copy link
Copy Markdown

@mattdurak mattdurak Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

; [](start = 53, length = 1)

nit: apostrophe #Resolved

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


In reply to: 591985218 [](ancestors = 591985218)

Comment thread src/utility.cpp Outdated
#endif
#endif

// have a stack local array of the addresses, don;t want to use malloc for this
Copy link
Copy Markdown

@mattdurak mattdurak Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// have [](start = 4, length = 8)

Probably good to have a comment here why we cache the real addresses/VirtualProtect is expensive (for "future developers") #Resolved

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added


In reply to: 591985755 [](ancestors = 591985755)

Comment thread src/utility.cpp
{
// we only process MAX_PATCH_ENTRY_COUNT, if we exceed it crash
// if this abort is ever hit, it means that MAX_PATCH_ENTRY_COUNT should be bumped up
abort();
Copy link
Copy Markdown

@mattdurak mattdurak Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

abort(); [](start = 12, length = 8)

Might be nice to log it with Report() #Resolved

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added


In reply to: 591986587 [](ancestors = 591986587)

Copy link
Copy Markdown

@mattdurak mattdurak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@mattdurak
Copy link
Copy Markdown

mattdurak commented Mar 11, 2021

We should tag the branch as 2.5.7 when this is merged and then upload a release with the installer binary https://github.com/Azure/vld/releases #Resolved

@dcristoloveanu
Copy link
Copy Markdown
Member Author

Will do


In reply to: 796335447 [](ancestors = 796335447)

@dcristoloveanu dcristoloveanu merged commit 6000883 into master Mar 11, 2021
@dcristoloveanu dcristoloveanu deleted the fix_AV_in_FindRealCode branch March 11, 2021 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants