Development TODO

Win32

Comments from Charles:

I only found one problem I'd call a show-stopper: no ACL on a pipe. As a result, current beta testers should be told there's a potential security problem with any pre-release based on this revision.

-C

Trunk at revision 350:

File: lib\win32\WinNamedPipeStream.cpp General points: NULL isn't the same as INVALID_HANDLE_VALUE (0 vs. 0xFFFFFFFF). bool isn't the same as BOOL (0 or 1 vs. DWORD used to return error codes). Yes, the people that did this need to be found and executed slowly and painfully, but this is how it is....

Function: ctor

mSocketHandle should be INVALID_HANDLE_VALUE (see below).

Function: Accept

This is insecure - anyone can connect to the pipe both locally and remotely. Since Box runs as LocalSystem (as a service) this is a Bad Idea (tm).

Function: Connect

Tests for NULL, not INVALID_HANDLE_VALUE. Will throw SocketAlreadyOpen if Connect fails and is retried. The pipe mode isn't set to message; it defaults to byte. This may cause interesting results.

Function: Read

Tests for NULL, not INVALID_HANDLE_VALUE. bool is not BOOL. ReadFile returns BOOL.

Function: Write

Tests for NULL, not INVALID_HANDLE_VALUE. bool is not BOOL. WriteFile returns BOOL. Code style: IMO would be better as a for() loop.

Function: Close Tests for NULL, not INVALID_HANDLE_VALUE. Sets mSocketHandle to NULL, not INVALID_HANDLE_VALUE.

File: lib\win32\enu.cpp

Function: RunTimer

This is pretty gross.... How many timers will ever be needed at one go? More than 31? If not, this can be _much_ better done with CreateEvent, CreateWaitableTimer and WaitForMultipleObjectsEx.

Function: EnableBackupRights

hToken isn't closed before exit.

Function: openfile

Use of MAX_PATH is inconsistent with
?\ Unicode syntax. Probably better to call GetCurrentDirectory directly.

Function: ourfstat

Call to GetFileSizeEx. Only available from Win2k onwards, and file size is provided by the preceding call to GetFileInformationByHandle anyway.

Function: OpenFileByNameUtf8

Use of MAX_PATH is inconsistent with
?\ Unicode syntax. Probably better to call GetCurrentDirectory directly.

Function: opendir

"delete dir->name" should be "delete [] dir->name"

Variable: tempbuff

Noted as not thread friendly. Prepend "declspec(thread)" to make TLS.