8223813: (aio) Iocp.getErrorMessage should drop trailing \r\n

[Brian Burkhalter]

https://bugs.openjdk.java.net/browse/JDK-8223813 

http://cr.openjdk.java.net/~bpb/8223813/webrev.00/ 


FormatMessage() and FormatMessageW() occur in a number of locations:

src/java.base/windows/native/libjli/java_md.c
src/java.base/windows/native/libnet/Inet4AddressImpl.c
src/java.base/windows/native/libjava/ProcessImpl_md.c
src/java.base/windows/native/libjava/jni_util_md.c
src/java.base/windows/native/libnio/ch/Iocp.c
src/java.base/windows/native/libnio/fs/WindowsNativeDispatcher.c
src/java.base/share/native/libzip/zlib/gzlib.c

Some of these already strip the terminal CRLF (or dot + CRLF) of the string 
populated by FormatMessage[W](). This patch would add removing them, if 
present, from

src/java.base/windows/native/libnet/Inet4AddressImpl.c
src/java.base/windows/native/libnio/ch/Iocp.c
src/java.base/windows/native/libnio/fs/WindowsNativeDispatcher.c

One question is whether it would be better just to consolidate this code into 
two methods for example in jni_uitl and call the methods from the other 
locations. There are already getLastErrorString() and getErrorString() for 
chars here.

Also, I am not sure how to test this effectively. The code passes all tiers 
as-is.

Thanks,

Brian

Re: 8223813: (aio) Iocp.getErrorMessage should drop trailing \r\n

[Roger Riggs]

Hi Brian,

Having the extraneous suffix consistently removed seems like a good thing.
Though I'm not sure what the best form of the utility function is:
 1) return the count of characters to remove
 2) just truncate the buffer.

There may be ways to force some of the errors that would lead to the 
truncation, but testing all uses will not be practical.


$.02, Roger

On 06/11/2019 01:56 PM, Brian Burkhalter wrote:

https://bugs.openjdk.java.net/browse/JDK-8223813
http://cr.openjdk.java.net/~bpb/8223813/webrev.00/ 



FormatMessage() and FormatMessageW() occur in a number of locations:

src/java.base/windows/native/libjli/java_md.c
src/java.base/windows/native/libnet/Inet4AddressImpl.c
src/java.base/windows/native/libjava/ProcessImpl_md.c
src/java.base/windows/native/libjava/jni_util_md.c
src/java.base/windows/native/libnio/ch/Iocp.c
src/java.base/windows/native/libnio/fs/WindowsNativeDispatcher.c
src/java.base/share/native/libzip/zlib/gzlib.c

Some of these already strip the terminal CRLF (or dot + CRLF) of the 
string populated by FormatMessage[W](). This patch would add removing 
them, if present, from


src/java.base/windows/native/libnet/Inet4AddressImpl.c
src/java.base/windows/native/libnio/ch/Iocp.c
src/java.base/windows/native/libnio/fs/WindowsNativeDispatcher.c

One question is whether it would be better just to consolidate this 
code into two methods for example in jni_uitl and call the methods 
from the other locations. There are already getLastErrorString() and 
getErrorString() for chars here.


Also, I am not sure how to test this effectively. The code passes all 
tiers as-is.


Thanks,

Brian

Re: 8223813: (aio) Iocp.getErrorMessage should drop trailing \r\n

[Brian Burkhalter]

Hi Roger,

Thanks for the $0.02.

> On Jun 11, 2019, at 12:14 PM, Roger Riggs  wrote:
> 
> Having the extraneous suffix consistently removed seems like a good thing.
> Though I'm not sure what the best form of the utility function is:
>  1) return the count of characters to remove
>  2) just truncate the buffer.

I am not sure either nor am I convinced that it would even be worth the effort. 
As mentioned there are already getLastErrorString() and getErrorString() for 
chars so I suppose the equivalent for WCHARs would need to be added for at 
least one of these.

> There may be ways to force some of the errors that would lead to the 
> truncation, but testing all uses will not be practical.

That’s what I thought.

Thanks,

Brian

Re: 8223813: (aio) Iocp.getErrorMessage should drop trailing \r\n

[Ivan Gerasimov]

Hi Brian!

Inet4AddressImpl.c:

1) There is an extra space after FormatMessage,

2) It is preexisting. The code doesn't check if FormatMessage failed to 
allocate the buffer.
It's not clear from the MSDN documentation, if the pointer to the buffer 
will be set to NULL upon the failure.
If it does not, then subsequent NET_ThrowNew(env, err, buf); 
LocalFree(buf); may hit uninitialized memory.

It would be more accurate to invoke them only if (n > 0).

3) It purely optional, but you may want to use the TEXT macro to append 
the L prefix to the character literals, if TCHAR is defined to be WCHAR:


 389 if (buf[n - 1] == TEXT('\n')) n--;
 390 if (buf[n - 1] == TEXT('\r')) n--;
 391 if (buf[n - 1] == TEXT('.')) n--;
 392 buf[n] = TEXT('\0');

It may make the compiler just a tiny bit happier :)

Everything else looks good to me.

With kind regards,
Ivan

On 6/11/19 10:56 AM, Brian Burkhalter wrote:

https://bugs.openjdk.java.net/browse/JDK-8223813
http://cr.openjdk.java.net/~bpb/8223813/webrev.00/ 



FormatMessage() and FormatMessageW() occur in a number of locations:

src/java.base/windows/native/libjli/java_md.c
src/java.base/windows/native/libnet/Inet4AddressImpl.c
src/java.base/windows/native/libjava/ProcessImpl_md.c
src/java.base/windows/native/libjava/jni_util_md.c
src/java.base/windows/native/libnio/ch/Iocp.c
src/java.base/windows/native/libnio/fs/WindowsNativeDispatcher.c
src/java.base/share/native/libzip/zlib/gzlib.c

Some of these already strip the terminal CRLF (or dot + CRLF) of the 
string populated by FormatMessage[W](). This patch would add removing 
them, if present, from


src/java.base/windows/native/libnet/Inet4AddressImpl.c
src/java.base/windows/native/libnio/ch/Iocp.c
src/java.base/windows/native/libnio/fs/WindowsNativeDispatcher.c

One question is whether it would be better just to consolidate this 
code into two methods for example in jni_uitl and call the methods 
from the other locations. There are already getLastErrorString() and 
getErrorString() for chars here.


Also, I am not sure how to test this effectively. The code passes all 
tiers as-is.


Thanks,

Brian


--
With kind regards,
Ivan Gerasimov

Re: 8223813: (aio) Iocp.getErrorMessage should drop trailing \r\n

[Brian Burkhalter]

Hi Ivan,

I updated the patch: http://cr.openjdk.java.net/~bpb/8223813/webrev.01/ 


Please see comments inline below.

> On Jun 11, 2019, at 5:06 PM, Ivan Gerasimov  wrote:
> 
> Inet4AddressImpl.c:
> 
> 1) There is an extra space after FormatMessage,

Fixed.

> 2) It is preexisting. The code doesn't check if FormatMessage failed to 
> allocate the buffer.
> It's not clear from the MSDN documentation, if the pointer to the buffer will 
> be set to NULL upon the failure.
> If it does not, then subsequent NET_ThrowNew(env, err, buf); LocalFree(buf); 
> may hit uninitialized memory.
> It would be more accurate to invoke them only if (n > 0).

I put “if (buf != NULL)” instead of “if (n > 0)”.

> 3) It purely optional, but you may want to use the TEXT macro to append the L 
> prefix to the character literals, if TCHAR is defined to be WCHAR:
> 
>  389 if (buf[n - 1] == TEXT('\n')) n--;
>  390 if (buf[n - 1] == TEXT('\r')) n--;
>  391 if (buf[n - 1] == TEXT('.')) n--;
>  392 buf[n] = TEXT('\0');
> 
> It may make the compiler just a tiny bit happier :)

So changed.

> Everything else looks good to me.

Thanks for the comments!

Brian

Re: 8223813: (aio) Iocp.getErrorMessage should drop trailing \r\n

[Ivan Gerasimov]

Thanks Brian!

The webrev looks fine to me.

I think that *most likely* the check if (buf != NULL) will work as 
expected:  buf will only be set to non-NULL value upon success.


However, the documentation for the function FormatMessage states:
"""
If the function fails, the return value is zero.
"""

So, my preference would be if (n > 0).

With kind regards,
Ivan



On 6/11/19 5:26 PM, Brian Burkhalter wrote:

Hi Ivan,

I updated the patch: 
http://cr.openjdk.java.net/~bpb/8223813/webrev.01/ 



Please see comments inline below.

On Jun 11, 2019, at 5:06 PM, Ivan Gerasimov 
mailto:ivan.gerasi...@oracle.com>> wrote:


Inet4AddressImpl.c:

1) There is an extra space after FormatMessage,


Fixed.

2) It is preexisting. The code doesn't check if FormatMessage failed 
to allocate the buffer.
It's not clear from the MSDN documentation, if the pointer to the 
buffer will be set to NULL upon the failure.
If it does not, then subsequent NET_ThrowNew(env, err, buf); 
LocalFree(buf); may hit uninitialized memory.

It would be more accurate to invoke them only if (n > 0).


I put “if (buf != NULL)” instead of “if (n > 0)”.

3) It purely optional, but you may want to use the TEXT macro to 
append the L prefix to the character literals, if TCHAR is defined to 
be WCHAR:


 389 if (buf[n - 1] == TEXT('\n')) n--;
 390 if (buf[n - 1] == TEXT('\r')) n--;
 391 if (buf[n - 1] == TEXT('.')) n--;
 392 buf[n] = TEXT('\0');

It may make the compiler just a tiny bit happier :)


So changed.


Everything else looks good to me.


Thanks for the comments!

Brian


--
With kind regards,
Ivan Gerasimov

Re: 8223813: (aio) Iocp.getErrorMessage should drop trailing \r\n

[Brian Burkhalter]

Hi Ivan,

I am perhaps beating a dead horse here, but how about this instead?

if (n > 0) {
NET_ThrowNew(env, err, buf);
LocalFree(buf);
} else {
NET_ThrowNew(env, err, "FormatMessage failed");
}

After all, an error *did* occur.

Thanks,

Brian

> On Jun 11, 2019, at 7:11 PM, Ivan Gerasimov  wrote:
> 
> The webrev looks fine to me.
> 
> I think that *most likely* the check if (buf != NULL) will work as expected:  
> buf will only be set to non-NULL value upon success.
> 
> However, the documentation for the function FormatMessage states:
> """
> If the function fails, the return value is zero.
> """
> 
> So, my preference would be if (n > 0).

Re: 8223813: (aio) Iocp.getErrorMessage should drop trailing \r\n

[Brian Burkhalter]

Actually, never mind, I am being completely lame here: both NET_ThrowNew() and 
the Windows function LocalFree() are robust to a NULL-valued buf so I think we 
can just remove the n > 0 or buf == NULL check altogether.

Sorry for the noise: I should have checked this first.

Thanks,

Brian

> On Jun 12, 2019, at 9:51 AM, Brian Burkhalter  
> wrote:
> 
> I am perhaps beating a dead horse here, but how about this instead?
> 
> if (n > 0) {
> NET_ThrowNew(env, err, buf);
> LocalFree(buf);
> } else {
> NET_ThrowNew(env, err, "FormatMessage failed");
> }
> 
> After all, an error *did* occur.

Re: 8223813: (aio) Iocp.getErrorMessage should drop trailing \r\n

[Ivan Gerasimov]

On 6/12/19 10:02 AM, Brian Burkhalter wrote:
Actually, never mind, I am being completely lame here: both 
NET_ThrowNew() and the Windows function LocalFree() are robust to a 
NULL-valued buf so I think we can just remove the n > 0 or buf == NULL 
check altogether.


That's true, assuming that you initialize buf = NULL and hopping that 
FormatMessage won't change buf upon failure.


I wish MSDN were a little bit more specific here.

I am fine with

1)
362 TCHAR *buf = NULL;

2)
unconditional
 395NET_ThrowNew(env, err, buf);
 396LocalFree(buf);

With kind regards,
Ivan


Sorry for the noise: I should have checked this first.

Thanks,

Brian

On Jun 12, 2019, at 9:51 AM, Brian Burkhalter 
mailto:brian.burkhal...@oracle.com>> wrote:


I am perhaps beating a dead horse here, but how about this instead?

if (n > 0) {
NET_ThrowNew(env, err, buf);
LocalFree(buf);
} else {
NET_ThrowNew(env, err, "FormatMessage failed");
}

After all, an error *did* occur.




--
With kind regards,
Ivan Gerasimov

Re: 8223813: (aio) Iocp.getErrorMessage should drop trailing \r\n

[Brian Burkhalter]

Hi Ivan,

> On Jun 12, 2019, at 4:03 PM, Ivan Gerasimov  wrote:
> 
> On 6/12/19 10:02 AM, Brian Burkhalter wrote:
>> Actually, never mind, I am being completely lame here: both NET_ThrowNew() 
>> and the Windows function LocalFree() are robust to a NULL-valued buf so I 
>> think we can just remove the n > 0 or buf == NULL check altogether.
>> 
> That's true, assuming that you initialize buf = NULL and hopping that 
> FormatMessage won't change buf upon failure.

True.

> I wish MSDN were a little bit more specific here.

Likewise.

> I am fine with
> 
> 1)
> 362 TCHAR *buf = NULL;
> 
> 2)
> unconditional
>  395NET_ThrowNew(env, err, buf);
>  396LocalFree(buf);

Barring objections I will check it in with the above code after the JDK 13 fork.

Thanks,

Brian