Security focused code review

I've been doing a lot of security-focused code reviewing lately, and got some insights. Especially when using C and below.
So, what to look for?

Inputs validation

I suppose it is trivial, but are the inputs validated correctly?

Time of check, time of use

Well, the inputs were validated, but are they under external control?
If they are, the attacker will be able to change them after they were validated, and cause havoc.
It is not really relevant in case of web requests, but when two entities communicate using shared memory it can happen.

Accessing single resource

When you have multiple threads and single resource, do note how the access is controlled.
Give special care for the initiation of the locking mechanism.
In general multi threading programming is hard, and the lower-level of the programming, the harder it gets.

Array Access

Every time an array is accessed, ask yourself if there is a possibility of out-of-bound access.

Using sizeof

Using sizeof can be tricky. If 'a' is an array, sizeof(a) can be either the size of the whole array, or equal to sizeof(void*). Then things won't behave correctly.

Handling faults

We all know that programmers program for the sunny day case. What happen when things go wrong? Is it even checked? handled?

Gamepad is accessible using...

A gamepad can be found and its state read by many interfaces.
Here are some that I saw used:

Detect and read state
XInput
Dinput
HID
Raw Input

Only Detect
WbemLocator
SetupDiEnumDeviceInfo and SetupDiGetDeviceInstanceId

WM_CHAR is not a real message

While investigating how to send capital-letter to an application, I found the message WM_CHAR.
Apparently, this message is not generated by the OS, but by TranslateMessage.

TranslateMessage get the WM_KEYDOWN, and if it is a character, check the modifiers, (such as shift, caps-lock, and other keys) and emit a WM_CHAR.

TranslateMessage is an kernel-level function, and check the status of the physical keys. so you can't mess with it. Also, because it is kernel-level, the message looks like the OS sent it.

How to make you game easy to be added to cloud gaming service

There are a few cloud gaming services active. If you want to sell them you game, and make it easy for them to use your game, here are a few tips:

1. Use the popular Direct3D version.
   Most of the games use 9, 9ex, 11.0/1.
2. Make rendering in a dedicated thread.
   Everything that can hinder rendering is bad. especially loading.
3. Always show something.
   Even if it is a stupid loading animation. because you can never know how long doing that operation will take.
4. Load data / save file / whatever in a separate thread.
5. Test your game under extreme CPU condition.
   Something running in the background?
   run with only 1 CPU?
   How many cores your game need to function? cores is money.
6. Test your game under extreme FPS conditions.
   Does your game runs OK in 20 fps? 100 fps?
7. How GPU intensive is your game?
   While heavy games will take half of the GPU time per frame, (=60 fps)
   There is no reason why your side-scroller will take more then quarter, or even a sixth of the GPU.
   GPU time is money. 
8. Test your game under heavy disk load.
   If the disk is very busy doing other things when the game is running, how is the experience?
9. Make your menus clean and simple.
   We are blocking the user from doing certain things, and if the menus are messy, moving and dynamic it will be very difficult.
10. Separate user-configs and system configs
    Things that user should be able to change: volume, subtitles.
    Should not be able to change: graphic settings, controller.
    Should be changes system-wise: language.
    Keep these in separate places.
    The Registry is considered separate places. 
11. Run the game under various graphic settings
    Full screen, windowed, vsync on, vsync off. and such. 

Activating Windows Loader debug messages

When you want to know what went wrong in loading the process.

My sources:
Entry Point Not Found, and other DLL Loading Problems
Microsoft System Journal, Under the Hood, September 1999

Basically, they highlight the usage of Windows Debugging Tools utility Global Flags.
It should be able to add a flag "Show Loader Snaps" to the target process.

Well, it doesn't do anything on my machine. So:

Go to
"HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Image File Execution Options"
Add a key with your EXE name, such as "ConsoleApplication1.exe" (without path)
Add a value to that key, Type: DWORD,  name: "GlobalFlag", value 2.

Now run the process under debugger.
Running and external debug messages collectors such as WinDbg or DebugView won't work.
But running it using Visual Studio or Ollydbg, the loader messages are showing in the output window.

Errors in Windows header files

From time to time I find errors in the WinAPI header files.
Usually, they are small errors, which can be very annoying in certain cases.

Example #1:
Some function definitions lack the DECLSPEC_IMPORT (which is a macro for DLL import/export tag)
Usually, it is not a problem. The compiler identify it as external, and the linker finds it in the WinAPI. But it means that the function pointer does not point directly to the DLL, but indirectly using stubs. On normal usage you won't notice that, but it causes problems with MSDetour library.

Example #2:
EnumPageFiles accept a callback of type PENUM_PAGE_CALLBACK.
But in the definition of that in Psapi.h, they forgot to add the CALLBACK modifier, that should make it to stdcall.
So, if you are using the function, at best you get a weird compilation error, or simply a crash.

SHGetKnownFolderPath: why is it linked with two jumps?

I noticed a weird behaviour when trying to detour SHGetKnownFolderPath, and investigaed a bit. So I worte a tiny program, and then loaded it in IDA.
The program:

#include "stdafx.h"
#include "windows.h"
#include "FileAPI.h"
#include "Shlobj.h"


int main()
{
    HANDLE hFile = CreateFileA(NULL, 0, 0, NULL, 0, 0, NULL);
    SHGetKnownFolderPath(FOLDERID_LocalAppDataLow, 0, NULL, NULL);
    return 0;
}

Looking in IDA, I see for CreateFileA:

call    ds:__imp__CreateFileA@28 ; CreateFileA(x,x,x,x,x,x,x)

Which takes me to:

.idata:0041B000 ; HANDLE __stdcall CreateFileA(LPCSTR lpFileName, DWORD dwDesiredAccess, DWORD dwShareMode, LPSECURITY_ATTRIBUTES lpSecurityAttributes, DWORD dwCreationDisposition, DWORD dwFlagsAndAttributes, HANDLE hTemplateFile)
.idata:0041B000                 extrn __imp__CreateFileA@28:dword ; CODE XREF: _main+2Ep
.idata:0041B000                                         ; DATA XREF: _main+2Er ...

On the other hand, for SHGetKnownFolderPath I see:

call    j__SHGetKnownFolderPath@16 ; SHGetKnownFolderPath(x,x,x,x)

Which redirect to:

; __stdcall SHGetKnownFolderPath(x, x, x, x)
j__SHGetKnownFolderPath@16 proc near
jmp     _SHGetKnownFolderPath@16 ; SHGetKnownFolderPath(x,x,x,x)
j__SHGetKnownFolderPath@16 endp

Which redirect to:

; __stdcall SHGetKnownFolderPath(x, x, x, x)
_SHGetKnownFolderPath@16 proc near
jmp     ds:__imp__SHGetKnownFolderPath@16 ; SHGetKnownFolderPath(x,x,x,x)
_SHGetKnownFolderPath@16 endp

Which redirect to:

.idata:0041B09C ; __declspec(dllimport) __stdcall SHGetKnownFolderPath(x, x, x, x)
.idata:0041B09C                 extrn __imp__SHGetKnownFolderPath@16:dword
.idata:0041B09C                                         ; DATA XREF: SHGetKnownFolderPath(x,x,x,x)r

So SHGetKnownFolderPath have two more jumps then CreateFileA. I did nothing to the project to make it do it. so why?

Update:

Removing Incremental Linking removed j__SHGetKnownFolderPath@16, but left _SHGetKnownFolderPath@16.

From the VS /INCREMENTAL documentation:

• May contain jump thunks to handle relocation of functions to new addresses

So we have half a solution, and another question: why does this setting effect only this DLL, and not other DLLs? (especially other system DLLs)

Update 2: Solved.
Found out that SHGetKnownFolderPath does not have a __declspec(dllimport) in the definition. Adding it made it be called identical to CreateFileA.
Without it, the compiler created a stub, the linker add a stub of its own for the incremental linking, and only then connected it to the DLL.