fix many bugs found by me, and reports by others in official repo.

[sonyps5201314]

fix many bugs found by me, and reports by others in official repo.

[ghost]

All CLA requirements met.

[sylveon]

I can't find an implementation for DetourUpdateAllOtherThreads
Please keep the source files in ASCII only
Your code should support VC6 (sadly)
Some of the issues already have pending PRs, for example #81
It was decided to prefer using CMake over VS as build system, so it would probably be best to remove your VS solution in favor of #48

Also nitpick: your memory allocation code invokes UB, you should use placement new.

[dtarditi]

It is is great to see this submissioj, thanks!

It is better to have separate commits for separate bugs. This makes it easier to test the changes and debug them later, if something breaks. I noticed that you are fixing some uninitialized variables, for example. Could you break this up into a commit per bug? Things like fixing uninitialized variables can all be part of one commit.

[sonyps5201314]

It is is great to see this submissioj, thanks!

It is better to have separate commits for separate bugs. This makes it easier to test the changes and debug them later, if something breaks. I noticed that you are fixing some uninitialized variables, for example. Could you break this up into a commit per bug? Things like fixing uninitialized variables can all be part of one commit.

I have been maintaining my own modified version of Detours for many years, but it has not been open source for a long time. Today I just released the code repo open source, so it may not be good to submit it step by step now, but I am sure there is no problem with this latest modified version. Since I've been using this latest code in my products for a while now, a lot of my software use my modified version of Detours. Including https://github.com/sonyps5201314/SocksCapEx2.

[sonyps5201314]

I can't find an implementation for DetourUpdateAllOtherThreads
Please keep the source files in ASCII only
Your code should support VC6 (sadly)
Some of the issues already have pending PRs, for example #81
It was decided to prefer using CMake over VS as build system, so it would probably be best to remove your VS solution in favor of #48

Also nitpick: your memory allocation code invokes UB, you should use placement new.

The VS solution is designed to make it easier for me and other users to see, modify, and debug the code.You can merge the changes, remove them and commit the result.
Use own memory allocation management functions rather than the CRT is for the sake of compatible DetourUpdateAllOtherThreads function, because the CRT lock memory allocation function, using DetourUpdateAllOtherThreads, if the current process exists the other thread, the thread will suspend operation, if the suspended thread at the moment have the CRT memory allocation locks, we continue call the CRT functions like 'new/malloc' will lead to a deadlock problem,This is a fix for #70

[sylveon]

Placement new does not involve a CRT lock, it just makes the code well defined by calling the T constructor.

[sonyps5201314]

your meaning is add a new/new[] operator overloading for some class? If this is your meaning, but there are many code use new/delete to manage BYTES memory:
rlpDlls = new NOTHROW LPCSTR [s_pHelper->nDlls]; in DetourFinishHelperProcess
Helper = (PDETOUR_EXE_HELPER) new NOTHROW BYTE[sizeof(DETOUR_EXE_HELPER) + cSize]; in AllocExeHelper
,these codes will use global new function, global new function is a simple call to malloc.
I decided to do this only when my actual tests found that there was a deadlock problem if I didn't use my own memory allocation function, rather than just extrapolating and guessing. All the official examples and all the code you can find on the web are basically calling DetourUpdateThread(GetCurrentThread()); So this problem didn't break out, and other HOOK libraries like Mhook have noticed this problem, so they also use their own memory management functions.

[sylveon]

No, I mean placement new.

T* thing = new (ptr) T;

This syntax calls the constructor of T at the address ptr and returns a pointer to the newly-constructed object. It does not involve allocation or overloading. Just casting the result of HeapAlloc is undefined behavior.

However, after looking at your code more closely, I notice that you had already done that, because I previously only looked at the cast. But one issue is that you should technically only access the new object through the return value of placement new, because that's the only safely derived pointer. I would suggest to rewrite the functions like this:

template<class T, class Arg>
T* DetourCreateObject(Arg arg)
{
	void* p = HeapAlloc(DetourGetHeap(), 0, sizeof(T));
	if (!p)
	{
		return NULL;
	}

	return ::new(p) T(arg);
}

Also, in DetourDestroyObject p should probably be taken by reference so that the final p = NULL; assignment is propagated to the caller.

[sonyps5201314]

No, I mean placement new.

T* thing = new (ptr) T;

This syntax calls the constructor of T at the address ptr and returns a pointer to the newly-constructed object. It does not involve allocation or overloading. Just casting the result of HeapAlloc is undefined behavior.

However, after looking at your code more closely, I notice that you had already done that, because I previously only looked at the cast. But one issue is that you should technically only access the new object through the return value of placement new, because that's the only safely derived pointer. I would suggest to rewrite the functions like this:

template<class T, class Arg>
T* DetourCreateObject(Arg arg)
{
	void* p = HeapAlloc(DetourGetHeap(), 0, sizeof(T));
	if (!p)
	{
		return NULL;
	}

	return ::new(p) T(arg);
}

Also, in DetourDestroyObject p should probably be taken by reference so that the final p = NULL; assignment is propagated to the caller.

I decide to not use reference to DetourDestroyObject parameter, Since I found Detours is well programmed, and its delete operation was mostly follow a reset pointer operation like below code, I did not set it to reference type in order to change Detours code as little as possible.

CImageImportFile::~CImageImportFile()
{
    if (m_pNextFile) {
        delete m_pNextFile;
        m_pNextFile = NULL;
    }
    if (m_pImportNames) {
        delete[] m_pImportNames;
        m_pImportNames = NULL;

If DetourDestroyObject' parameter was set to reference type, the codes like 'm_pImportNames = NULL;' should be remove for the sake of efficiency, and I found many similar code existed. so I decide to do not use reference at that moment, you are official people , so you have many high rights to decides to remove these junk code if change p to reference type.

[sonyps5201314]

No, I mean placement new.

T* thing = new (ptr) T;

This syntax calls the constructor of T at the address ptr and returns a pointer to the newly-constructed object. It does not involve allocation or overloading. Just casting the result of HeapAlloc is undefined behavior.

However, after looking at your code more closely, I notice that you had already done that, because I previously only looked at the cast. But one issue is that you should technically only access the new object through the return value of placement new, because that's the only safely derived pointer. I would suggest to rewrite the functions like this:

template<class T, class Arg>
T* DetourCreateObject(Arg arg)
{
	void* p = HeapAlloc(DetourGetHeap(), 0, sizeof(T));
	if (!p)
	{
		return NULL;
	}

	return ::new(p) T(arg);
}

Also, in DetourDestroyObject p should probably be taken by reference so that the final p = NULL; assignment is propagated to the caller.

I create DetourCreateObject function like ATL collections classes, so I think it is no problem at that mement, the code like
the below code in ATL of VS2019 and so on.

template< typename E, class ETraits >
typename CAtlList< E, ETraits >::CNode* CAtlList< E, ETraits >::NewNode(
	/* _In_ */ INARGTYPE element,
	_In_opt_ CNode* pPrev,
	_In_opt_ CNode* pNext)
{
	GetFreeNode();

	CNode* pNewNode = m_pFree;
	CNode* pNextFree = m_pFree->m_pNext;

	::new( pNewNode ) CNode( element );

	m_pFree = pNextFree;
	pNewNode->m_pPrev = pPrev;
	pNewNode->m_pNext = pNext;
	m_nElements++;
	ATLASSUME( m_nElements > 0 );

	return( pNewNode );
}

[sonyps5201314]

@sylveon and the VS solution is not conflicting, it only call the below code:

SET DETOURS_TARGET_PROCESSOR=XXX
cd ..
nmake

but exist it will friend for edit, modify and debug code in Detours.

[sylveon]

copy paste mistake?

[sonyps5201314]

yes, 😁, it fix now, and not important because in comment.