search

microsoft / Detours

Detours is a software package for monitoring and instrumenting API calls on Windows. It is distributed in source code form.
MIT License
5.01k stars 979 forks source link

Fix Compiler Warnings #192

Closed rchildre3 closed 3 years ago

rchildre3 commented 3 years ago

MultiByteToWideChar never returns negative so it can be safely casted to an unsigned value

Closes #188 and #189

Microsoft Reviewers: Open in CodeFlow
bgianfo commented 3 years ago

Thanks @rchildre3 for the contribution! Sorry the delay in merging.

sonyps5201314 commented 3 years ago

@rchildre3 If MultiByteToWideChar is hooked by the user, it may return a negative number, which is why I want to check whether it is less than or equal to 0 instead of equal to 0: cchWrittenWideChar <= 0 The correct reform should be:cchWrittenWideChar >= (int)ARRAYSIZE(szDllName)

bgianfo commented 3 years ago

That's true... if the user hooks it they could have bugs which don't obey the published spec... Why does this great feedback always seem to come after the PR's are merged and never before? 😄

@rchildre3 will you be able to submit a patch for this? If not I'll just revert the change for now...

sonyps5201314 commented 3 years ago

@rchildre3 in detours.cpp, Instead of changing the original code：cbTarget > sizeof(pTrampoline->rbCode) to the new code：pTrampoline->cbRestore > sizeof(pTrampoline->rbCode), why not directly modify the variable type? ：ULONG cbTarget

sonyps5201314 commented 3 years ago

That's true... if the user hooks it they could have bugs which don't obey the published spec... Why does this great feedback always seem to come after the PR's are merged and never before?

@rchildre3 will you be able to submit a patch for this? If not I'll just revert the change for now...

Because I only pull the latest code, if I find that my code and original intentions have been modified, will I discover the possible problems. Before this, no one at me went to review the relevant code.😂

rchildre3 commented 3 years ago

I yield to you @sonyps5201314 , it seems as though you may have more material knowledge in this section of code, though a comment about the check you mentioned would have been helpful in understanding the signededness of the variables in this section

sonyps5201314 commented 3 years ago

Thanks for your attention.

bgianfo commented 3 years ago

@rchildre3 FYI the change was reverted...