[Box Backup-dev] Win32 code review

[Charles Lecklider]

Since we're coming up on 0.10 I thought I'd have a quick look through
the Win32 stuff in trunk; please note my use of the word "quick".

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.