Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2071273004:
Call GetAdaptersAddresses in the way recommended by MSDN. (Closed)
|
Created:
4 years, 6 months ago by Taylor_Brandstetter Modified:
4 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCall GetAdaptersAddresses in the way recommended by MSDN.
Previously, we were calling GetAdaptersAddresses once to get the
required buffer size, then a second time to fill the buffer. This
method is "strongly discouraged" by MSDN. They recommend an initial
size of 15KB to avoid calling GetAdaptersAddresses multiple times.
We also were not calling GetAdaptersAddresses in a loop. In between
two calls to GetAdaptersAddresses, the required size may increase.
Instead of treating this as an error, we now increase our buffer
size and try again.
BUG=616908, 534463
Committed: https://crrev.com/e5a246f582215340d3c71022a87d4c152a8269df
Cr-Commit-Position: refs/heads/master@{#401123}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Responding to review comments. #Patch Set 3 : Fixing typo. #Patch Set 4 : Changing size to 15000, not 15360. #Messages
Total messages: 18 (7 generated)
deadbeef@chromium.org changed reviewers: + pauljensen@chromium.org
PTAL. This fixes an issue where WebRTC stops working after a network change event.
https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... File net/base/network_interfaces_win.cc (right): https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:30: const int MAX_GETADAPTERSADDRESSES_TRIES = 100; how about moving this constant into the function? https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:30: const int MAX_GETADAPTERSADDRESSES_TRIES = 100; 100 seems like a lot. I imagine 10 should be plenty? https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:205: // Use an initial buffer size of 15KB, as recommended by MSDN. can you provide a link to this recommendation? https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:213: std::unique_ptr<char[]> buf; can we type this as IP_ADAPTER_ADDRESSES*? https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:218: buf.reset(new char[len]); it would be nice if the first 15k buffer could be on the stack so we don't always have to allocate and free 15k, but it's not a big deal.
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
Please add 534463 to BUG=. It looks like the same root cause.
Description was changed from ========== Call GetAdaptersAddresses in the way recommended by MSDN. Previously, we were calling GetAdaptersAddresses once to get the required buffer size, then a second time to fill the buffer. This method is "strongly discouraged" by MSDN. They recommend an initial size of 15KB to avoid calling GetAdaptersAddresses multiple times. We also were not calling GetAdaptersAddresses in a loop. In between two calls to GetAdaptersAddresses, the required size may increase. Instead of treating this as an error, we now increase our buffer size and try again. BUG=616908 ========== to ========== Call GetAdaptersAddresses in the way recommended by MSDN. Previously, we were calling GetAdaptersAddresses once to get the required buffer size, then a second time to fill the buffer. This method is "strongly discouraged" by MSDN. They recommend an initial size of 15KB to avoid calling GetAdaptersAddresses multiple times. We also were not calling GetAdaptersAddresses in a loop. In between two calls to GetAdaptersAddresses, the required size may increase. Instead of treating this as an error, we now increase our buffer size and try again. BUG=616908,534463 ==========
Added link to other bug as requested. https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... File net/base/network_interfaces_win.cc (right): https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:30: const int MAX_GETADAPTERSADDRESSES_TRIES = 100; On 2016/06/20 11:32:49, pauljensen wrote: > 100 seems like a lot. I imagine 10 should be plenty? Done. I thought to make it a large number so that this issue doesn't resurface later, but you're likely right. https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:30: const int MAX_GETADAPTERSADDRESSES_TRIES = 100; On 2016/06/20 11:32:49, pauljensen wrote: > how about moving this constant into the function? Done. https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:205: // Use an initial buffer size of 15KB, as recommended by MSDN. On 2016/06/20 11:32:49, pauljensen wrote: > can you provide a link to this recommendation? Done. https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:213: std::unique_ptr<char[]> buf; On 2016/06/20 11:32:49, pauljensen wrote: > can we type this as IP_ADAPTER_ADDRESSES*? I think that may be confusing, because the buffer's size will not be a multiple of `sizeof(IP_ADAPTER_ADDRESSES)`. We would need to type it as `IP_ADAPTER_ADDRESSES[]` so that the array delete operator is used, and call `buf.reset(reinterpret_cast<IP_ADAPTER_ADDRESSES*>(new char[len]))`. If you do prefer that, I'll go ahead. https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:218: buf.reset(new char[len]); On 2016/06/20 11:32:49, pauljensen wrote: > it would be nice if the first 15k buffer could be on the stack so we don't > always have to allocate and free 15k, but it's not a big deal. Done.
https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... File net/base/network_interfaces_win.cc (right): https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:213: std::unique_ptr<char[]> buf; On 2016/06/21 17:06:05, Taylor_Brandstetter wrote: > On 2016/06/20 11:32:49, pauljensen wrote: > > can we type this as IP_ADAPTER_ADDRESSES*? > > I think that may be confusing, because the buffer's size will not be a multiple > of `sizeof(IP_ADAPTER_ADDRESSES)`. We would need to type it as > `IP_ADAPTER_ADDRESSES[]` so that the array delete operator is used, and call > `buf.reset(reinterpret_cast<IP_ADAPTER_ADDRESSES*>(new char[len]))`. If you do > prefer that, I'll go ahead. I assume IP_ADAPTER_ADDRESSES is POD so it's silly that it has to be an array, but I guess this is what unique_ptr wants. To be on the safe side and to avoid the ugly cast, I'd allocate it with: buf.reset(new IP_ADAPTER_ADDRESSES[(len + sizeof(IP_ADAPTER_ADDRESSES) - 1)/sizeof(IP_ADAPTER_ADDRESSES)]); or maybe define a ROUND_UP macro and use that to make it a little easier to read.
https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... File net/base/network_interfaces_win.cc (right): https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:213: std::unique_ptr<char[]> buf; On 2016/06/21 17:20:10, pauljensen wrote: > On 2016/06/21 17:06:05, Taylor_Brandstetter wrote: > > On 2016/06/20 11:32:49, pauljensen wrote: > > > can we type this as IP_ADAPTER_ADDRESSES*? > > > > I think that may be confusing, because the buffer's size will not be a > multiple > > of `sizeof(IP_ADAPTER_ADDRESSES)`. We would need to type it as > > `IP_ADAPTER_ADDRESSES[]` so that the array delete operator is used, and call > > `buf.reset(reinterpret_cast<IP_ADAPTER_ADDRESSES*>(new char[len]))`. If you do > > prefer that, I'll go ahead. > > I assume IP_ADAPTER_ADDRESSES is POD so it's silly that it has to be an array, > but I guess this is what unique_ptr wants. > > To be on the safe side and to avoid the ugly cast, I'd allocate it with: > buf.reset(new IP_ADAPTER_ADDRESSES[(len + sizeof(IP_ADAPTER_ADDRESSES) - > 1)/sizeof(IP_ADAPTER_ADDRESSES)]); > > or maybe define a ROUND_UP macro and use that to make it a little easier to > read. Well... It depends on your definition of POD. It contains pointers to several linked lists and variable size strings, and is itself the head node of a linked list. So it would seem odd to me to declare an array when the underlying data is a linked list. Someone may try to access an element other than [0] and get undefined behavior.
lgtm with one comment https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... File net/base/network_interfaces_win.cc (right): https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:206: ULONG len = 15360; Actually I think Windows may want you to use 15000 like their example shows: https://msdn.microsoft.com/en-us/library/windows/desktop/aa365915(v=vs.85).aspx https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:213: std::unique_ptr<char[]> buf; On 2016/06/21 19:50:51, Taylor_Brandstetter wrote: > On 2016/06/21 17:20:10, pauljensen wrote: > > On 2016/06/21 17:06:05, Taylor_Brandstetter wrote: > > > On 2016/06/20 11:32:49, pauljensen wrote: > > > > can we type this as IP_ADAPTER_ADDRESSES*? > > > > > > I think that may be confusing, because the buffer's size will not be a > > multiple > > > of `sizeof(IP_ADAPTER_ADDRESSES)`. We would need to type it as > > > `IP_ADAPTER_ADDRESSES[]` so that the array delete operator is used, and call > > > `buf.reset(reinterpret_cast<IP_ADAPTER_ADDRESSES*>(new char[len]))`. If you > do > > > prefer that, I'll go ahead. > > > > I assume IP_ADAPTER_ADDRESSES is POD so it's silly that it has to be an array, > > but I guess this is what unique_ptr wants. > > > > To be on the safe side and to avoid the ugly cast, I'd allocate it with: > > buf.reset(new IP_ADAPTER_ADDRESSES[(len + sizeof(IP_ADAPTER_ADDRESSES) - > > 1)/sizeof(IP_ADAPTER_ADDRESSES)]); > > > > or maybe define a ROUND_UP macro and use that to make it a little easier to > > read. > > Well... It depends on your definition of POD. It contains pointers to several > linked lists and variable size strings, and is itself the head node of a linked > list. So it would seem odd to me to declare an array when the underlying data is > a linked list. Someone may try to access an element other than [0] and get > undefined behavior. I think the POD definition is pretty clear; isn't it part of the C++ spec? I think all this Windows stuff is from the days of C so it has to be POD. Anyhow, I guess the current char[] and cast is fine as this isn't really an array of anything sane.
https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... File net/base/network_interfaces_win.cc (right): https://codereview.chromium.org/2071273004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:206: ULONG len = 15360; On 2016/06/21 20:32:05, pauljensen wrote: > Actually I think Windows may want you to use 15000 like their example shows: > https://msdn.microsoft.com/en-us/library/windows/desktop/aa365915(v=vs.85).aspx Done, good catch.
The CQ bit was checked by deadbeef@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pauljensen@chromium.org Link to the patchset: https://codereview.chromium.org/2071273004/#ps60001 (title: "Changing size to 15000, not 15360.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2071273004/60001
Message was sent while issue was closed.
Description was changed from ========== Call GetAdaptersAddresses in the way recommended by MSDN. Previously, we were calling GetAdaptersAddresses once to get the required buffer size, then a second time to fill the buffer. This method is "strongly discouraged" by MSDN. They recommend an initial size of 15KB to avoid calling GetAdaptersAddresses multiple times. We also were not calling GetAdaptersAddresses in a loop. In between two calls to GetAdaptersAddresses, the required size may increase. Instead of treating this as an error, we now increase our buffer size and try again. BUG=616908,534463 ========== to ========== Call GetAdaptersAddresses in the way recommended by MSDN. Previously, we were calling GetAdaptersAddresses once to get the required buffer size, then a second time to fill the buffer. This method is "strongly discouraged" by MSDN. They recommend an initial size of 15KB to avoid calling GetAdaptersAddresses multiple times. We also were not calling GetAdaptersAddresses in a loop. In between two calls to GetAdaptersAddresses, the required size may increase. Instead of treating this as an error, we now increase our buffer size and try again. BUG=616908,534463 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Call GetAdaptersAddresses in the way recommended by MSDN. Previously, we were calling GetAdaptersAddresses once to get the required buffer size, then a second time to fill the buffer. This method is "strongly discouraged" by MSDN. They recommend an initial size of 15KB to avoid calling GetAdaptersAddresses multiple times. We also were not calling GetAdaptersAddresses in a loop. In between two calls to GetAdaptersAddresses, the required size may increase. Instead of treating this as an error, we now increase our buffer size and try again. BUG=616908,534463 ========== to ========== Call GetAdaptersAddresses in the way recommended by MSDN. Previously, we were calling GetAdaptersAddresses once to get the required buffer size, then a second time to fill the buffer. This method is "strongly discouraged" by MSDN. They recommend an initial size of 15KB to avoid calling GetAdaptersAddresses multiple times. We also were not calling GetAdaptersAddresses in a loop. In between two calls to GetAdaptersAddresses, the required size may increase. Instead of treating this as an error, we now increase our buffer size and try again. BUG=616908,534463 Committed: https://crrev.com/e5a246f582215340d3c71022a87d4c152a8269df Cr-Commit-Position: refs/heads/master@{#401123} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e5a246f582215340d3c71022a87d4c152a8269df Cr-Commit-Position: refs/heads/master@{#401123} |
