The Wayback Machine - https://web.archive.org/web/20201216141840/https://github.com/microsoft/Detours/pull/140
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

Fix can not createwith some protected exe #140

Open

Conversation

@sonyps5201314
Copy link
Contributor

@sonyps5201314 sonyps5201314 commented Sep 4, 2020

fix can not use createwith api to start some unnormal exe, like chinese online、commercial、prorected game dnf.exe https://dnf.qq.com/, but this exe can start run by explorer.

}
++pImageImport;
}
OutputDebugString(TEXT("[This PE file has an import table, but the import table size is marked as 0. This is an error.")

This comment has been minimized.

@bgianfo

bgianfo Sep 4, 2020
Member

We should use DETOUR_TRACE instead of OutputDebugStrring?

This comment has been minimized.

@sonyps5201314

sonyps5201314 Sep 5, 2020
Author Contributor

DETOUR_TRACE can not output any message if no define DETOUR_DEBUG, I use OutputDebugString to force output message for tell user to pay attention to this is a unnormal exe.

This comment has been minimized.

@sylveon

sylveon Sep 5, 2020

Should finish with a ] if it starts with [. Also OutputDebugString doesn't automatically append a new line, so you want to add \n at the end of the string.

This comment has been minimized.

@sonyps5201314

sonyps5201314 Sep 5, 2020
Author Contributor

\r\n is in next line, this is a const string concat operation.

This comment has been minimized.

@bgianfo

bgianfo Sep 6, 2020
Member

DETOUR_TRACE can not output any message if no define DETOUR_DEBUG, I use OutputDebugString to force output message for tell user to pay attention to this is a unnormal exe.

So the users is going to just happen to be running under a debugger?
OutputDebugString only outputs a message if a debugger is attached.

It's not really detours job to emit messages as library.
I think it would make more sense to just return a rich
error code in LastError, and let the caller handle that case
explicitly.

This comment has been minimized.

@sonyps5201314

sonyps5201314 Sep 6, 2020
Author Contributor

Dbgview and many debug tools can capture the debug output statements of the application without being debugged.
In fact, it doesn't matter whether the user notices this message, it's like an MFC running warning.
like

		TRACE(traceAppMsg, 0, _T("Warning: calling DestroyWindow in CWnd::~CWnd; ")
		   _T("OnDestroy or PostNcDestroy in derived class will not be called.\n"));

in

CWnd::~CWnd()
{
	if (m_hWnd != NULL &&
		this != (CWnd*)&wndTop && this != (CWnd*)&wndBottom &&
		this != (CWnd*)&wndTopMost && this != (CWnd*)&wndNoTopMost)
	{
		TRACE(traceAppMsg, 0, _T("Warning: calling DestroyWindow in CWnd::~CWnd; ")
		   _T("OnDestroy or PostNcDestroy in derived class will not be called.\n"));
		DestroyWindow();
	}

The user cannot handle it because the only way to deal with it is to modify the PE file on the disk, but this file may contain a digital signature, and the modification may be illegal, and it only has a problem when it is launched through Detours' createwith API. The user uses Explorer. exe starts it, or directly calls the CreateProcess function to start it, there is no problem, these are enough to prove a defect of Detours.
The only improvement is that calling OutputDebugString in a non-debugging state may modify the value of LastError of the current thread, which has been fixed in the latest submission.

Copy link
Member

@bgianfo bgianfo left a comment

Thanks for the contribution!

It looks like you might have mistakenly included all of the other commits from your other PRs here?
Can you pair this down to only the issue you describe in your pull request description?

@bgianfo bgianfo added the bug label Sep 4, 2020
…se online、commercial、prorected game dnf.exe https://dnf.qq.com/, but this exe can start run by explorer.
@sonyps5201314 sonyps5201314 force-pushed the sonyps5201314:fix_can_not_createwith_some_prorected_exe branch from 62b7fbf to b3d45f4 Sep 5, 2020
@sonyps5201314
Copy link
Contributor Author

@sonyps5201314 sonyps5201314 commented Sep 5, 2020

@bgianfo, I have rebased to only include this commit.

src/uimports.cpp Outdated Show resolved Hide resolved
@sonyps5201314 sonyps5201314 requested a review from bgianfo Sep 6, 2020
src/uimports.cpp Outdated Show resolved Hide resolved
src/uimports.cpp Outdated Show resolved Hide resolved
src/uimports.cpp Outdated Show resolved Hide resolved
src/uimports.cpp Outdated Show resolved Hide resolved
src/uimports.cpp Outdated Show resolved Hide resolved
while(ImageImport.Name)
{
inh.IMPORT_DIRECTORY.Size+=sizeof(IMAGE_IMPORT_DESCRIPTOR);
if(!ReadProcessMemory(hProcess,pImageImport,&ImageImport,sizeof(ImageImport),NULL))

This comment has been minimized.

@bgianfo

bgianfo Sep 6, 2020
Member

I think you could simplify this code by switching this to a do-while loop, that way the initial ReadProcessMemory doesn't need to be special cased.

This comment has been minimized.

@sonyps5201314

sonyps5201314 Sep 6, 2020
Author Contributor

I don't quite understand your expression, and here ImageImport.Name is a judgment condition, so I think it is more appropriate to use a while-do loop.

This comment has been minimized.

@halx99

halx99 Sep 28, 2020

The loop maybe better:

for (int count=0;;++count) {
        if (!ReadProcessMemory(hProcess, pImageImport, &ImageImport, sizeof(ImageImport), NULL)) {
            DETOUR_TRACE(("ReadProcessMemory failed: %u\n", GetLastError()));;
            goto finish;
        }
        if (ImageImport.Name) {
            inh.IMPORT_DIRECTORY.Size += sizeof(IMAGE_IMPORT_DESCRIPTOR);  
            ++pImageImport;
            continue;
        }
        if(count) inh.IMPORT_DIRECTORY.Size += sizeof(IMAGE_IMPORT_DESCRIPTOR);  
        break;
}

This comment has been minimized.

This comment has been minimized.

@sonyps5201314

sonyps5201314 Sep 28, 2020
Author Contributor

Although the code becomes able to get the correct result after you update and fix it, but at the same time it is difficult to read. In my original code, only run inh.IMPORT_DIRECTORY.Size+=sizeof(IMAGE_IMPORT_DESCRIPTOR); under the condition of ImageImport.Name , and you changed this code but first executed inh.IMPORT_DIRECTORY.Size+=sizeof(IMAGE_IMPORT_DESCRIPTOR); and ignored the conditions of ImageImport.Name. It seems that it is just to make the result correct, so I think it breaks the readability and there is no need to modify it.

This comment has been minimized.

@halx99

halx99 Sep 28, 2020

why, it's easy to unstand, just calc size until the name is null

This comment has been minimized.

@halx99

halx99 Sep 29, 2020

Update again, the latest code logical is equal to your original code

This comment has been minimized.

@sonyps5201314

sonyps5201314 Sep 29, 2020
Author Contributor

OK, now the code has been adjusted to be more in line with official recommendations.

@bgianfo bgianfo changed the title Fix can not createwith some prorected exe Fix can not createwith some protected exe Sep 6, 2020
…ty checks are performed.
@sonyps5201314 sonyps5201314 force-pushed the sonyps5201314:fix_can_not_createwith_some_prorected_exe branch from bc81266 to 107d601 Sep 6, 2020
@sonyps5201314 sonyps5201314 requested a review from bgianfo Sep 6, 2020
@sonyps5201314 sonyps5201314 force-pushed the sonyps5201314:fix_can_not_createwith_some_prorected_exe branch from 518a1a4 to 9d58115 Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.