Add a new function for getting a deterministic machine ID

chrome/browser/prefs/tracked/device_id_win.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/prefs/tracked/device_id.h"
+
+#include <cstddef>
+#include <cwchar>

[gab, 2015/01/20 15:34:12]

I don't think you need these two headers.

[alito, 2015/01/26 16:32:48]

<cstddef> was for NULL. But probably also defined in windows.h. But <cwchar> is
for wchar_t. I don't see it being defined in any other headers directly. Should
I not include it?

[gab, 2015/01/26 19:40:46]

I've never seen anyone include <cwchar>, from what I understand wchar_t is a
native type in MSVC but I may be wrong.

@grt?

[alito, 2015/01/26 21:01:10]

I've removed <cwchar>. It is not normally included in chromium code it seems.

[grt (UTC plus 2), 2015/01/27 17:06:22]

New code should use nullptr.
wchar_t is a fundamental type in the language, so no header is needed.

[gab, 2015/01/27 18:27:40]

Yes, but in this case it's using Windows APIs whose documentation still uses
NULL (sure it's the same thing in practice).

So do you think we should use nullptr even when calling into Windows APIs (i.e.
for the pointer-checking benefit)? Or stick to the documentation?

[grt (UTC plus 2), 2015/01/27 19:48:59]

Yup, use nullptr rather than NULL when calling a Windows API that takes a
pointer to something. Use nullptr everywhere possible.

[alito, 2015/01/27 20:56:55]

Done.

+
+#include <intSafe.h>  // For DWORD.

[gab, 2015/01/20 15:34:12]

DWORD implicitly comes with windows.h AFAIK (I've never seen anyone include
intsafe.h for it at least).

[alito, 2015/01/26 16:32:48]

Done.

+#include <sddl.h>  // For ConvertSidToStringSidW.

[gab, 2015/01/20 15:34:12]

s/ConvertSidToStringSidW/ConvertSidToStringSidA

[alito, 2015/01/26 16:32:48]

Done.

+#include <windows.h>

[gab, 2015/01/20 15:34:12]

We always put windows.h first (this is more or less the only exception to
"always alpha sort the headers").

We put the definitions in the following order:

<interface header>

<C headers> (alpha-sorted, but windows.h first if needed)
(optional empty line -- I like having it but it's not required AFAICT)
<C++ headers> (alpha-sorted)

<project headers> (alpha-sorted)

So in this case (with the headers you have here if we weren't changing them):

<windows.h>
<intsafe.h>
<sddl.h>

<cstddef>
<cwchar>

<base/macros.h>
...


(see this article for more details:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord...)

[alito, 2015/01/26 16:32:48]

Done.

+
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+
+bool GetDeterministicMachineSpecificId(std::string* machine_id) {

[gab, 2015/01/20 15:34:13]

Looks like there is a GetUserSidString() method in base/win; it would probably
make sense to have a general implementation of a GetMachineSidString() there as
well (used by both this code and RLZ) since this is non-trivial code.

[alito, 2015/01/26 16:32:48]

As we discussed offline, we will wait with this refactoring until later.

+  if (!machine_id)
+    return false;

[gab, 2015/01/20 15:34:13]

Instead do:
DCHECK(machine_id);
to enforce the contract that |machine_id| should always be provided (and mention
that contract in the header).

[alito, 2015/01/26 16:32:48]

Done.

+
+  wchar_t computer_name[MAX_COMPUTERNAME_LENGTH + 1] = {0};

[gab, 2015/01/20 15:34:12]

{0} is the same as {} except that you explicitly set the first element to 0 and
the rest is default initialized.

{} is preferable here (@grt: I know you're an advocate of this, is this still
the recommendation?)

[alito, 2015/01/26 16:32:48]

Done.

[grt (UTC plus 2), 2015/01/27 17:06:22]

It's still my preference.

[alito, 2015/01/27 20:56:55]

Acknowledged.

+  DWORD computer_name_size = arraysize(computer_name);
+
+  if (!::GetComputerNameW(computer_name, &computer_name_size))
+    return false;
+
+  DWORD sid_size = SECURITY_MAX_SID_SIZE;
+  char sid_buffer[SECURITY_MAX_SID_SIZE];
+  SID* sid = reinterpret_cast<SID*>(sid_buffer);
+  DWORD domain_size = 128;  // Will expand if needed.
+  scoped_ptr<wchar_t[]> domain_buffer(new wchar_t[domain_size]);
+  SID_NAME_USE sid_name_use;
+
+  if (!::LookupAccountNameW(NULL, computer_name, sid, &sid_size,
+                            domain_buffer.get(), &domain_size,
+                            &sid_name_use)) {
+    // If the initial size of domain_buffer was too small, the
+    // required size is now found in domain_size. Resize and try
+    // again.
+    if (::GetLastError() != ERROR_INSUFFICIENT_BUFFER)
+      return false;
+
+    domain_buffer.reset(new wchar_t[domain_size]);
+    if (!::LookupAccountNameW(NULL, computer_name, sid, &sid_size,
+                              domain_buffer.get(), &domain_size,

[gab, 2015/01/20 15:34:12]

|domain_buffer| is optional and we don't use its value, change it to NULL and
|domain_size| to 0

This also makes it unnecessary to call this a second time if it fails the first
time IIUC.

[alito, 2015/01/26 16:32:48]

The documentation does not mention this explicitly, but in fact, the call to
|LookupAccountNameW| will return false and set the error code to
|ERROR_INSUFFICIENT_BUFFER| if |domain_buffer| is NULL. This is despite the fact
that the parameter is annotated as |_Out_opt_|. So, in order to ensure that the
call was successfull and we did in fact obtain a valid SID, we need to make sure
|domain_buffer| can hold the domain.

[gab, 2015/01/26 19:40:45]

I see, please add comments to that effect to avoid a future reader spending some
time figuring this out yet again.

[alito, 2015/01/26 21:01:10]

Done.

+                              &sid_name_use))
+      return false;

[gab, 2015/01/20 15:34:12]

Wrap in {}

(always wrap in {} unless both the conditional and the body are one-liners)

[alito, 2015/01/26 16:32:48]

Done.

+  }
+

[gab, 2015/01/20 15:34:12]

DCHECK_EQ(SID_NAME_USE::SidTypeComputer, sid_name_use);

to confirm that we're using this API the right way and are indeed getting the
machine's SID

[alito, 2015/01/26 16:32:48]

As we discussed offline, this actually fails consistently. See comments in the
updated code. Because of this confusion in the API I've added a run-time test
instead and will return FAILURE if the value of |sid_name_use| is unexpected.

+  char* sid_string = NULL;
+  if (!::ConvertSidToStringSidA(sid, &sid_string))
+    return false;
+
+  *machine_id = sid_string;
+  ::LocalFree(sid_string);
+
+  return true;
+}