Fix uninitialized reads in DnsConfigServiceWin tests.

Fix uninitialized reads in DnsConfigServiceWin tests.

BUG=486575
Committed: https://crrev.com/9d04c6ad2ac23314e6dd5d9f9bb6029146fcb4e0
Cr-Commit-Position: refs/heads/master@{#329499}

[benwells, 2015-05-11 03:37:11]

benwells@chromium.org changed reviewers:
+ thakis@chromium.org

[benwells, 2015-05-11 03:37:12]

This turned out to be yuckier than I thought it would. Let me know if there is a
cleaner approach.

[Nico, 2015-05-11 04:08:30]

Thanks for looking at my mess!

https://codereview.chromium.org/1132333003/diff/1/net/dns/dns_config_service_...
File net/dns/dns_config_service_win_unittest.cc (right):

https://codereview.chromium.org/1132333003/diff/1/net/dns/dns_config_service_...
net/dns/dns_config_service_win_unittest.cc:261: { true, L"primary.dns.suffix" },
Huh, I thought that using a brace initializer for a struct zero-initializes all
fields after the listed ones to 0 (like `mystruct a = {};` sets all fields to
0). Is that not the case here? Why not?

If not, I'd just add a few `{ { false }, { false } }` lines in these blocks
rather than add all these automatic initializers.

[benwells, 2015-05-11 05:12:38]

https://codereview.chromium.org/1132333003/diff/1/net/dns/dns_config_service_...
File net/dns/dns_config_service_win_unittest.cc (right):

https://codereview.chromium.org/1132333003/diff/1/net/dns/dns_config_service_...
net/dns/dns_config_service_win_unittest.cc:261: { true, L"primary.dns.suffix" },
On 2015/05/11 04:08:29, Nico wrote:
> Huh, I thought that using a brace initializer for a struct zero-initializes
all
> fields after the listed ones to 0 (like `mystruct a = {};` sets all fields to
> 0). Is that not the case here? Why not?
> 
> If not, I'd just add a few `{ { false }, { false } }` lines in these blocks
> rather than add all these automatic initializers.

The problem was elsewhere, with these changes fallout from the actual fix. See
next comment for details.

https://codereview.chromium.org/1132333003/diff/1/net/dns/dns_config_service_...
net/dns/dns_config_service_win_unittest.cc:486:
internal::ConvertSettingsToDnsConfig(settings, &config));
This was the line causing the error that I investigated. The problem is that
settings.policy_search_list.set is not initialized, because the new default
initializer for DnsSystemSettings initializes policy_search_list with
policy_search_list().

As RegString has no default values / no constructor this leaves set
uninitialized.

The actual fix for this was to add the "= false" to RegString and RegDword. The
rest of the change was required to make this test compile.

[Nico, 2015-05-11 17:12:47]

On 2015/05/11 05:12:38, benwells wrote:
>
https://codereview.chromium.org/1132333003/diff/1/net/dns/dns_config_service_...
> File net/dns/dns_config_service_win_unittest.cc (right):
> 
>
https://codereview.chromium.org/1132333003/diff/1/net/dns/dns_config_service_...
> net/dns/dns_config_service_win_unittest.cc:261: { true, L"primary.dns.suffix"
},
> On 2015/05/11 04:08:29, Nico wrote:
> > Huh, I thought that using a brace initializer for a struct zero-initializes
> all
> > fields after the listed ones to 0 (like `mystruct a = {};` sets all fields
to
> > 0). Is that not the case here? Why not?
> > 
> > If not, I'd just add a few `{ { false }, { false } }` lines in these blocks
> > rather than add all these automatic initializers.
> 
> The problem was elsewhere, with these changes fallout from the actual fix. See
> next comment for details.
> 
>
https://codereview.chromium.org/1132333003/diff/1/net/dns/dns_config_service_...
> net/dns/dns_config_service_win_unittest.cc:486:
> internal::ConvertSettingsToDnsConfig(settings, &config));
> This was the line causing the error that I investigated. The problem is that
> settings.policy_search_list.set is not initialized, because the new default
> initializer for DnsSystemSettings initializes policy_search_list with
> policy_search_list().

Ah. Is it possible to say `policy_search_list{}` instead to keep aggegate
initialization instead of value initialization for that member? (Or maybe
`policy_search_list{false}` or just say `policy_search_list.set = false;` in the
ctor body? Then all the other stuff isn't needed.

> 
> As RegString has no default values / no constructor this leaves set
> uninitialized.
> 
> The actual fix for this was to add the "= false" to RegString and RegDword.
The
> rest of the change was required to make this test compile.

[benwells, 2015-05-12 00:27:09]

On 2015/05/11 17:12:47, Nico (mostly OOO this monday) wrote:
> On 2015/05/11 05:12:38, benwells wrote:
> >
>
https://codereview.chromium.org/1132333003/diff/1/net/dns/dns_config_service_...
> > File net/dns/dns_config_service_win_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/1132333003/diff/1/net/dns/dns_config_service_...
> > net/dns/dns_config_service_win_unittest.cc:261: { true,
L"primary.dns.suffix"
> },
> > On 2015/05/11 04:08:29, Nico wrote:
> > > Huh, I thought that using a brace initializer for a struct
zero-initializes
> > all
> > > fields after the listed ones to 0 (like `mystruct a = {};` sets all fields
> to
> > > 0). Is that not the case here? Why not?
> > > 
> > > If not, I'd just add a few `{ { false }, { false } }` lines in these
blocks
> > > rather than add all these automatic initializers.
> > 
> > The problem was elsewhere, with these changes fallout from the actual fix.
See
> > next comment for details.
> > 
> >
>
https://codereview.chromium.org/1132333003/diff/1/net/dns/dns_config_service_...
> > net/dns/dns_config_service_win_unittest.cc:486:
> > internal::ConvertSettingsToDnsConfig(settings, &config));
> > This was the line causing the error that I investigated. The problem is that
> > settings.policy_search_list.set is not initialized, because the new default
> > initializer for DnsSystemSettings initializes policy_search_list with
> > policy_search_list().
> 
> Ah. Is it possible to say `policy_search_list{}` instead to keep aggegate
> initialization instead of value initialization for that member? (Or maybe
> `policy_search_list{false}` or just say `policy_search_list.set = false;` in
the
> ctor body? Then all the other stuff isn't needed.

How's this? I don't think policy_search_list{false} is allowed due to uniform
initialization syntax being banned, but this basically does the same thing.

> 
> > 
> > As RegString has no default values / no constructor this leaves set
> > uninitialized.
> > 
> > The actual fix for this was to add the "= false" to RegString and RegDword.
> The
> > rest of the change was required to make this test compile.

[Nico, 2015-05-12 00:32:17]

lgtm, but lbtm with the suggestion.

Again thank you very much for doing this for me.

https://codereview.chromium.org/1132333003/diff/20001/net/dns/dns_config_serv...
File net/dns/dns_config_service_win.cc (right):

https://codereview.chromium.org/1132333003/diff/20001/net/dns/dns_config_serv...
net/dns/dns_config_service_win.cc:447: append_to_multi_label_name({}),
Hm, isn't this uniform initialization too?
Nicos-MacBook-Pro-4:src thakis$ clang -c test.cc -std=c++11
Nicos-MacBook-Pro-4:src thakis$ cat test.cc
struct A {
  int a, b;
};

struct B {
  B();
  A a_;
};

B::B() : a_({}) {
}
Nicos-MacBook-Pro-4:src thakis$ clang -c test.cc
test.cc:10:13: error: expected expression
B::B() : a_({}) {
            ^
1 error generated.
Nicos-MacBook-Pro-4:src thakis$ clang -c test.cc -std=c++11
# works


Maybe best to just set all the "set"s to false in the body explicitly :-/

[benwells, 2015-05-12 04:15:12]

benwells@chromium.org changed reviewers:
+ rsleevi@chromium.org

[benwells, 2015-05-12 04:15:37]

On 2015/05/12 00:32:17, Nico (mostly OOO this monday) wrote:
> lgtm, but lbtm with the suggestion.
> 
> Again thank you very much for doing this for me.
> 
>
https://codereview.chromium.org/1132333003/diff/20001/net/dns/dns_config_serv...
> File net/dns/dns_config_service_win.cc (right):
> 
>
https://codereview.chromium.org/1132333003/diff/20001/net/dns/dns_config_serv...
> net/dns/dns_config_service_win.cc:447: append_to_multi_label_name({}),
> Hm, isn't this uniform initialization too?
> Nicos-MacBook-Pro-4:src thakis$ clang -c test.cc -std=c++11
> Nicos-MacBook-Pro-4:src thakis$ cat test.cc
> struct A {
>   int a, b;
> };
> 
> struct B {
>   B();
>   A a_;
> };
> 
> B::B() : a_({}) {
> }
> Nicos-MacBook-Pro-4:src thakis$ clang -c test.cc
> test.cc:10:13: error: expected expression
> B::B() : a_({}) {
>             ^
> 1 error generated.
> Nicos-MacBook-Pro-4:src thakis$ clang -c test.cc -std=c++11
> # works
> 
> 
> Maybe best to just set all the "set"s to false in the body explicitly :-/

Ah .. ok. That's a shame.... done.

+rsleevi for owners review

[Nico, 2015-05-12 16:35:19]

lgtm, thanks.

[Ryan Sleevi, 2015-05-12 20:36:48]

blah, LGTM

[Nico, 2015-05-12 20:43:55]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2015-05-12 20:44:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1132333003/40001

[commit-bot: I haz the power, 2015-05-12 21:42:39]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-12 21:43:24]

Patchset 3 (id:??) landed as
https://crrev.com/9d04c6ad2ac23314e6dd5d9f9bb6029146fcb4e0
Cr-Commit-Position: refs/heads/master@{#329499}