Work performed on various issues

[ioannis-e]

The highlights of the changes made are as follows:

• Install a hook on LdrpCallInitRoutine() or the relevant nt loader code to cover all Windows XP to Windows 10, x86 and x64 platforms in order to Refresh Modules before the dll entry point. The current implementation Refreshes the Modules after the dll entry point which in effect does not detect any allocations made on dll entry point.
• Fix #9519, #9859, #10544, use LoaderLock to serialize any calls which load additional libraries or require access to the Loader Lock.
• Fix #6359, #10553, fix crashes and failure to register COM dlls where Visual Leak Detector is unloaded before IMalloc interface is released.
• Optimize _GetProcAddress and _GetProcAddressForCaller to properly test for ordinal vs function name
• Append Visual Studio 2015/2013/2012/2010/2008 symbols cache directory where multiple VS versions are installed on the same system.
• Add option to skip reporting crt startup allocations as memory leaks
  • for debug runtime (where debug crt allocations are not properly identied) and
  • for release runtime (by excluding certain startup crt initialisation modules), this may have an effect in performance.
• Improve the vld.ini searching from additional locations listed below in order, and fix installer issue where it would not set the vld.ini path in registry for x64 platform:
  • current directory
  • directory where VLDDLL resides
  • directory where executable resides
  • path specified in registry HKCU
  • path specified in registry HKLM
• All other commits resolve compiler warnings and improvements to the code following PVS-Studio analysis.
• Clean up of solutions and project files
• Improvements and additions to test cases

I tried to keep commits independed of each other as much as possible, to make review easier.

I would suggest to cherypick as much as possible into master in order to rebase this PR and leave us with only the commits that need discussion.

[ioannis-e]

Do you want any changes made from this point on in one or more new commits or edit the relevant commits in place? What's best for review?

[KindDragon]

You can make changes in new commit and then rebase before we will merge this PR.

[ioannis-e]

Super, we lost the entire review because i had to rebase master due to 778a1e9

In any case I have resolved the performance degradation issue.
So you are good to go again.

[ioannis-e]

I'll have time this weekend to resolve any new review points so we can finally finish this.
Only things left for me to do is:

• merge review commits with original commits once you give me the ok
• run a 4 spaces formatting at the end, to have consistent code format. separate PR

[KindDragon]

VLDGetLeaksCount() return 11 for me

[ioannis-e]

VS2015 ? Debug / Release ? (I am still on VS2013)
AppVeyor reports 8 leaks in the Release test for both Win32 and x64.
Can you post the full report so we can check if they are crt leaks

[KindDragon]

I am calling VLDReportLeaks() before return from main
VS2015 Debug return 9 https://gist.github.com/KindDragon/fcb0757383efd6b4018a
VS2015 Debug_StaticCrt return 12 https://gist.github.com/KindDragon/06b1dc5e13f62956eb0c

[KindDragon]

Your code crash on x64 (Windows 10) when call FreeLibrary
Exception thrown at 0x00007FF8870B3D6D (ntdll.dll) in vld_main.exe: 0xC0000005: Access violation reading location 0xFFFFFFFFFFFFFFFF.

[KindDragon]

Please cheery pick this commit 8d155b7 . Results with TestUnload - https://ci.appveyor.com/project/KindDragon/vld/build/71/job/2089w3s2taa31ltu/tests

[KindDragon]

and ComTest doesn't register on x64

[ioannis-e]

I have fixed

• crash on x64 (Windows 10) when call FreeLibrary
• ComTest doesn't register on x64
• vld_main additional CRT leaks on VS2015
• improvements to vld_unload test

[ioannis-e]

With the modification made to the project files, the vs14 solution is loadable in vs12, the only thing is that the platform should be changed within VS, I have made the platform a default setting for all configurations, so when it changes it changes for all configurations.

In this respect i guess we dont need to keep vs12 and vs14 solutions.
Would you agree to remove the vs12 solutions and projects ?

[KindDragon]

Great work

Would you agree to remove the vs12 solutions and projects ?

I'm using VS12 solution to check vld with VS2013 toolset on local Teamcity. Therefore, it is still necessary to me.

[ioannis-e]

Ok, i'll keep them and i will make the solutions and projects the same appart from the toolset.

[ioannis-e]

Updated projects and solutions
Added PerUserREdirection to ComTest
Added StaticCrt to ComTest (lets see if anything else pops up on appveyor)

If there is nothing else, i am ready !

[ioannis-e]

Builds successfully again!
So let me know how to proceed.

[KindDragon]

So everything is ok. If you have already done, I merge PR.

[ioannis-e]

Do you want me to squash any commits ? or leave them as they are ?
I'll make another PR for source formatting

[ioannis-e]

The following should be resolved:
https://vld.codeplex.com/workitem/10548
https://vld.codeplex.com/workitem/10588
https://vld.codeplex.com/workitem/10587

[KindDragon]

If not hard please squash commits that fix other commits like 53277cb with b491ca2, 69f8936 with b38b1e0, 3a8a03b with de78327
Please also add comment to de78327 why we should skip count CRT_USE_TYPE(crtheader->use) == CRT_USE_IGNORE

[ioannis-e]

I am testing as well in my repo.
https://ci.appveyor.com/project/ioannis-e/vld/build/3
i'd suggest to use a develop branch to test stuff to keep master clean?

[ioannis-e]

Appveyor: error MSB8020: The build tools for v110_xp (Platform Toolset = 'v110_xp') cannot be found.
Maybe ask support how to install ?

Environment: VldStackWalkMethod=safe, Solution=vld_vs12.sln; Configuration: Release_StaticCrt; Platform: Win32

Seems to be deadlocking on appveyor but runs ok localy ...

[KindDragon]

Seems to be deadlocking on appveyor but runs ok localy ...

With StackWalkMethod safe?

[ioannis-e]

wait, let me test safe locally... and i'll report back

[ioannis-e]

Yes just run dynamic_app test with StackWalkMethod=safe and it runs ok locally, but deadlocks in appveyor.
Can you run the same test as well ?

[ioannis-e]

https://ci.appveyor.com/project/ioannis-e/vld/build/3

So it only fails on vs12 Release_StaticCrt Win32/x64 in safe mode
So how is the test different between vs14 and vs12 ?

[KindDragon]

Hang for me

[ioannis-e]

On which windows are you ?

[ioannis-e]

Can you add a LoaderLock ll; in SafeCallstack::getStackTrace

[KindDragon]

win 10

[KindDragon]

Works for me. Waiting for appveyor https://ci.appveyor.com/project/KindDragon/vld/build/99

[KindDragon]

Now Environment: VldStackWalkMethod=safe, Solution=vld_vs12.sln; Configuration: Debug_VldRelease_StaticCrt; Platform: Win32 hang

[KindDragon]

LdrLock deadlock with crt heap lock

[ioannis-e]

Damn, thread syncronisations don't work... i thought i got them working (back to the drawing board)

[ioannis-e]

See if you like ioannis-e/vld@357f91a and whether it works for you as well ?
https://ci.appveyor.com/project/ioannis-e/vld/build/7

I temporarily removed VS2012 appveyour tests since they kept failing. Can you build/run in VS2012/VS2010 at all ?
I am interested to see whether vld_main_test and vld_unload exhibit different behaviour.

[KindDragon]

Can you build/run in VS2012/VS2010 at all ?

I can not configure build under VS2010

[ioannis-e]

I meant locally not on appveyor.
I have VS2010 on x86, but the Batch Build... feature is broken, and i cannot test all configurations.
I want to confirm that isCrtStartupModule() reports correctly in older versions.

Did you get any feedback from appveyor support regarding v110 toolset ?

[KindDragon]

I meant locally not on appveyor.

I meant this too

[KindDragon]

See if you like ioannis-e/vld@357f91a

I don't know how I fill about this changes :-) Too many critical sections and locks.
Also, help for DbgHelp doesn't guarantee that your changes will work.

All DbgHelp functions, are single threaded. Therefore, calls from more than one thread to this function will likely result in unexpected behavior or memory corruption. To avoid this, you must synchronize all concurrent calls from more than one thread to this function.

But you use 4 CS.

[ioannis-e]

@KindDragon I understand, so let me explain my reasoning.

There were already 2 locks in code g_symbolLock and g_imageLock so I guess there was a reasoning behind having two locks instead of just one lock.

Using g_symbolLock with StackWalk64 caused a deadlock in safe and StaticCrt mode.
I added 2 more one for StackWalk64 and one for EnumerateLoadedModulesW64
I kept the same lock for all other Sym* functions.

Also I used the wrapper functions in order to avoid unecessary blocking or neigbouring code by the locks.

Regarding DbgHelp help its unclear to me whether all functions of DbgHelp need to be syncronised between threads or whether calls to the same function need to be syncronised between threads, I went with the latter and haven't identified any corruption yet...

[KindDragon]

I also fix deadlock in Magic commit :-) 9902f53 https://ci.appveyor.com/project/KindDragon/vld/build/100
But I do not remember why I added heapMapLock earlier

[KindDragon]

heapMapLock was added not so long ago b9a53cd instead of g_stackWalkLock

[KindDragon]

Also I used the wrapper functions in order to avoid unecessary blocking or neigbouring code by the locks.

But frequent entry/exit to critical section is slower than once to enter before loop.

[KindDragon]

How you think if I partially revert b9a53cd ("CS g_stackWalkLock merged with g_heapMapLock").
I think this should fix deadlock

[ioannis-e]

You see upto that commit each DbgHelp function had its own locker and all other Sym* functions had one locker, which is basically what i did but instead put them in a class.

So I guess you should go back to g_stackWalkLock for StackWalk64 and probably add one more lock for EnumerateModulesW64.

What I also considered is that StackWalk64 and EnumerateModulesW64 are the most time consuming functions

I would also suggest the following changes
m_modulesLock does not need to block IsModulePatched because that function acquires other locks that could lead to deadlock
ioannis-e@357f91a#diff-a895db1a9b34fcae7efbc9c48edaf1c4L726
and
Also I think nobody likes IsBadReadPtr :)
ioannis-e@357f91a#diff-a895db1a9b34fcae7efbc9c48edaf1c4L723

[ioannis-e]

I finally managed to build under VS2010.

I have an issue with _heap_term() where in VS2010 it calls HeapDestroy while in VS2013 and VS2015 it does not.

This causes some StaticCrt tests ie dynamic_app and vld_unload to fail in VS2010 as the leaks are reported before destroying the heap (in order to perform the memory dump) and the heap map is erased, and are not taken into account when calling GetLeakCount.

I was wandering whether you would accept on HeapDestroy in addition to reporting the leaks immediately, to keep the heap info (if leaks are found) and resolve the stacks in order to be able to report the leaks when vld unloads without a memory dump.

I'm currently working on some changes to see how it goes.

[KindDragon]

Did you get any feedback from appveyor support regarding v110 toolset ?

They use VS Express and it doesn't contain MFC

[KindDragon]

I finally managed to build under VS2010.

VS2012 build failed for Environment: VldStackWalkMethod=fast, Solution=vld_vs11_wo_mfc.sln, GTEST_FILTER=-.Mfc; Configuration: Release_StaticCrt; Platform: Win32
https://ci.appveyor.com/project/KindDragon/vld/build/119

[KindDragon]

This causes some StaticCrt tests ie dynamic_app and vld_unload to fail in VS2010 as the leaks are reported before destroying the heap (in order to perform the memory dump) and the heap map is erased, and are not taken into account when calling GetLeakCount.

Maybe just change leaks count for this configuration (will simply be known difference in behavior). Someday we will remove support for VS2010