RFR[10]: 8187658: Bigger buffer for GetAdaptersAddresses

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR[10]: 8187658: Bigger buffer for GetAdaptersAddresses

Ivan Gerasimov
Hello!

When retrieving information about network interfaces on Windows we make
up to 2 attempts to call GetAdaptersAddresses().

It was reported that in very rare cases it may not be sufficient, and
even the second attempt can fail with ERROR_BUFFER_OVERFLOW.

I suggest that we follow the recommendation given in MSDN [1]: increase
the initial buffer size to 15K and do up to 3 attempts to call
GetAdaptersAddresses().

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8187658
WEBREV: http://cr.openjdk.java.net/~igerasim/8187658/00/webrev/

Would you please help review the fix?

[1]
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365915(v=vs.85).aspx

--
With kind regards,
Ivan Gerasimov

Reply | Threaded
Open this post in threaded view
|

Re: RFR[10]: 8187658: Bigger buffer for GetAdaptersAddresses

Ivan Gerasimov
Ping.

Please review the proposed change at your convenience.

The fix will greatly reduce the possibility of a need to reallocate the
buffer to retrieve the results (something that the documentation
strongly suggests to avoid), and, if such reallocation still occurs to
be necessary, will increase number of retries.

With kind regards,
Ivan


On 9/19/17 10:50 AM, Ivan Gerasimov wrote:

> Hello!
>
> When retrieving information about network interfaces on Windows we
> make up to 2 attempts to call GetAdaptersAddresses().
>
> It was reported that in very rare cases it may not be sufficient, and
> even the second attempt can fail with ERROR_BUFFER_OVERFLOW.
>
> I suggest that we follow the recommendation given in MSDN [1]:
> increase the initial buffer size to 15K and do up to 3 attempts to
> call GetAdaptersAddresses().
>
> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8187658
> WEBREV: http://cr.openjdk.java.net/~igerasim/8187658/00/webrev/
>
> Would you please help review the fix?
>
> [1]
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa365915(v=vs.85).aspx
>

--
With kind regards,
Ivan Gerasimov

Reply | Threaded
Open this post in threaded view
|

Re: RFR[10]: 8187658: Bigger buffer for GetAdaptersAddresses

roger riggs
Hi Ivan,

Looks ok to me.

I don't see a reason to skimp on the additional size since it is
allocated and then freed immediately.
The increment might as well be as big as the initial size.

I don't see a reason to retry if the buffer gets close to ULONG_MAX; I'd
just break the for loop
and let it fail. (but the new code is just doing what the old code did
and retry even if the buffer did not increase).

Also, please introduce a constant for the number of retries; it will
make it clearer
and not need to hard code it in two places.
(I would probably have coded the retry count counting down to zero but
its the same effect).

Regards, Roger


On 9/25/2017 1:03 PM, Ivan Gerasimov wrote:

> Ping.
>
> Please review the proposed change at your convenience.
>
> The fix will greatly reduce the possibility of a need to reallocate
> the buffer to retrieve the results (something that the documentation
> strongly suggests to avoid), and, if such reallocation still occurs to
> be necessary, will increase number of retries.
>
> With kind regards,
> Ivan
>
>
> On 9/19/17 10:50 AM, Ivan Gerasimov wrote:
>> Hello!
>>
>> When retrieving information about network interfaces on Windows we
>> make up to 2 attempts to call GetAdaptersAddresses().
>>
>> It was reported that in very rare cases it may not be sufficient, and
>> even the second attempt can fail with ERROR_BUFFER_OVERFLOW.
>>
>> I suggest that we follow the recommendation given in MSDN [1]:
>> increase the initial buffer size to 15K and do up to 3 attempts to
>> call GetAdaptersAddresses().
>>
>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8187658
>> WEBREV: http://cr.openjdk.java.net/~igerasim/8187658/00/webrev/
>>
>> Would you please help review the fix?
>>
>> [1]
>> https://msdn.microsoft.com/en-us/library/windows/desktop/aa365915(v=vs.85).aspx
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR[10]: 8187658: Bigger buffer for GetAdaptersAddresses

Ivan Gerasimov
Thank you Roger for review!


On 9/25/17 11:47 AM, Roger Riggs wrote:
> Hi Ivan,
>
> Looks ok to me.
>
> I don't see a reason to skimp on the additional size since it is
> allocated and then freed immediately.
> The increment might as well be as big as the initial size.
>
Right.  Let's use the same value of 15K, which reduces the number of
variables to operate with.

> I don't see a reason to retry if the buffer gets close to ULONG_MAX;
> I'd just break the for loop
> and let it fail. (but the new code is just doing what the old code did
> and retry even if the buffer did not increase).
>
If len is close to ULONG_MAX, it is still larger value of len returned
by the previous call to GetAdaptersAddresses (otherwise we wouldn't have
gotten ERROR_BUFFER_OVERFLOW.)
So, if we're in the loop, there's no way the buffer will not increase
(unless there's a failure due to OOM, of course.)

> Also, please introduce a constant for the number of retries; it will
> make it clearer
> and not need to hard code it in two places.
Done!

Would you please take a look at the updated webrev:

http://cr.openjdk.java.net/~igerasim/8187658/01/webrev/

With kind regards,
Ivan

> (I would probably have coded the retry count counting down to zero but
> its the same effect).
>
> Regards, Roger
>
>
> On 9/25/2017 1:03 PM, Ivan Gerasimov wrote:
>> Ping.
>>
>> Please review the proposed change at your convenience.
>>
>> The fix will greatly reduce the possibility of a need to reallocate
>> the buffer to retrieve the results (something that the documentation
>> strongly suggests to avoid), and, if such reallocation still occurs
>> to be necessary, will increase number of retries.
>>
>> With kind regards,
>> Ivan
>>
>>
>> On 9/19/17 10:50 AM, Ivan Gerasimov wrote:
>>> Hello!
>>>
>>> When retrieving information about network interfaces on Windows we
>>> make up to 2 attempts to call GetAdaptersAddresses().
>>>
>>> It was reported that in very rare cases it may not be sufficient,
>>> and even the second attempt can fail with ERROR_BUFFER_OVERFLOW.
>>>
>>> I suggest that we follow the recommendation given in MSDN [1]:
>>> increase the initial buffer size to 15K and do up to 3 attempts to
>>> call GetAdaptersAddresses().
>>>
>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8187658
>>> WEBREV: http://cr.openjdk.java.net/~igerasim/8187658/00/webrev/
>>>
>>> Would you please help review the fix?
>>>
>>> [1]
>>> https://msdn.microsoft.com/en-us/library/windows/desktop/aa365915(v=vs.85).aspx
>>>
>>
>
>

--
With kind regards,
Ivan Gerasimov

Reply | Threaded
Open this post in threaded view
|

Re: RFR[10]: 8187658: Bigger buffer for GetAdaptersAddresses

roger riggs
Hi Ivan,

Looks fine,

Thanks, Roger

On 9/25/2017 4:58 PM, Ivan Gerasimov wrote:
Thank you Roger for review!


On 9/25/17 11:47 AM, Roger Riggs wrote:
Hi Ivan,

Looks ok to me.

I don't see a reason to skimp on the additional size since it is allocated and then freed immediately.
The increment might as well be as big as the initial size.

Right.  Let's use the same value of 15K, which reduces the number of variables to operate with.

I don't see a reason to retry if the buffer gets close to ULONG_MAX; I'd just break the for loop
and let it fail. (but the new code is just doing what the old code did and retry even if the buffer did not increase).

If len is close to ULONG_MAX, it is still larger value of len returned by the previous call to GetAdaptersAddresses (otherwise we wouldn't have gotten ERROR_BUFFER_OVERFLOW.)
So, if we're in the loop, there's no way the buffer will not increase (unless there's a failure due to OOM, of course.)

Also, please introduce a constant for the number of retries; it will make it clearer
and not need to hard code it in two places.
Done!

Would you please take a look at the updated webrev:

http://cr.openjdk.java.net/~igerasim/8187658/01/webrev/

With kind regards,
Ivan

(I would probably have coded the retry count counting down to zero but its the same effect).

Regards, Roger


On 9/25/2017 1:03 PM, Ivan Gerasimov wrote:
Ping.

Please review the proposed change at your convenience.

The fix will greatly reduce the possibility of a need to reallocate the buffer to retrieve the results (something that the documentation strongly suggests to avoid), and, if such reallocation still occurs to be necessary, will increase number of retries.

With kind regards,
Ivan


On 9/19/17 10:50 AM, Ivan Gerasimov wrote:
Hello!

When retrieving information about network interfaces on Windows we make up to 2 attempts to call GetAdaptersAddresses().

It was reported that in very rare cases it may not be sufficient, and even the second attempt can fail with ERROR_BUFFER_OVERFLOW.

I suggest that we follow the recommendation given in MSDN [1]: increase the initial buffer size to 15K and do up to 3 attempts to call GetAdaptersAddresses().

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8187658
WEBREV: http://cr.openjdk.java.net/~igerasim/8187658/00/webrev/

Would you please help review the fix?

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa365915(v=vs.85).aspx






Reply | Threaded
Open this post in threaded view
|

Re: RFR[10]: 8187658: Bigger buffer for GetAdaptersAddresses

Vyom Tewari
In reply to this post by Ivan Gerasimov


On Tuesday 26 September 2017 02:28 AM, Ivan Gerasimov wrote:

> Thank you Roger for review!
>
>
> On 9/25/17 11:47 AM, Roger Riggs wrote:
>> Hi Ivan,
>>
>> Looks ok to me.
>>
>> I don't see a reason to skimp on the additional size since it is
>> allocated and then freed immediately.
>> The increment might as well be as big as the initial size.
>>
> Right.  Let's use the same value of 15K, which reduces the number of
> variables to operate with.
>
>> I don't see a reason to retry if the buffer gets close to ULONG_MAX;
>> I'd just break the for loop
>> and let it fail. (but the new code is just doing what the old code
>> did and retry even if the buffer did not increase).
>>
> If len is close to ULONG_MAX, it is still larger value of len returned
> by the previous call to GetAdaptersAddresses (otherwise we wouldn't
> have gotten ERROR_BUFFER_OVERFLOW.)
> So, if we're in the loop, there's no way the buffer will not increase
> (unless there's a failure due to OOM, of course.)
>
>> Also, please introduce a constant for the number of retries; it will
>> make it clearer
>> and not need to hard code it in two places.
> Done!
>
> Would you please take a look at the updated webrev:
>
> http://cr.openjdk.java.net/~igerasim/8187658/01/webrev/
looks good to me , although i am not the reviewer.
Vyom

>
> With kind regards,
> Ivan
>
>> (I would probably have coded the retry count counting down to zero
>> but its the same effect).
>>
>> Regards, Roger
>>
>>
>> On 9/25/2017 1:03 PM, Ivan Gerasimov wrote:
>>> Ping.
>>>
>>> Please review the proposed change at your convenience.
>>>
>>> The fix will greatly reduce the possibility of a need to reallocate
>>> the buffer to retrieve the results (something that the documentation
>>> strongly suggests to avoid), and, if such reallocation still occurs
>>> to be necessary, will increase number of retries.
>>>
>>> With kind regards,
>>> Ivan
>>>
>>>
>>> On 9/19/17 10:50 AM, Ivan Gerasimov wrote:
>>>> Hello!
>>>>
>>>> When retrieving information about network interfaces on Windows we
>>>> make up to 2 attempts to call GetAdaptersAddresses().
>>>>
>>>> It was reported that in very rare cases it may not be sufficient,
>>>> and even the second attempt can fail with ERROR_BUFFER_OVERFLOW.
>>>>
>>>> I suggest that we follow the recommendation given in MSDN [1]:
>>>> increase the initial buffer size to 15K and do up to 3 attempts to
>>>> call GetAdaptersAddresses().
>>>>
>>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8187658
>>>> WEBREV: http://cr.openjdk.java.net/~igerasim/8187658/00/webrev/
>>>>
>>>> Would you please help review the fix?
>>>>
>>>> [1]
>>>> https://msdn.microsoft.com/en-us/library/windows/desktop/aa365915(v=vs.85).aspx
>>>>
>>>
>>
>>
>