RFR [8055421]: (fs) bad error handling in java.base/unix/native/libnio/fs/UnixNativeDispatcher.c

[Ivan Gerasimov]

Thank you Martin for finding another bug in the same file!

And I think I found yet another one in fclose wrapper:
The man page clearly states that after fclose is called, the stream 
should not be accessed again, including other calls to fclose.

Would you please help review the updated webrev?

WEBREV: http://cr.openjdk.java.net/~igerasim/8055421/1/webrev/

Sincerely yours,
Ivan


On 21.08.2014 20:23, Martin Buchholz wrote:
> Of course Ivan's change is a good bug fix.
>
> Meta comments:
>
> Y'all really need to work on common C-level infrastructure.  E.g. I 
> see 15 definitions of RESTARTABLE in the jdk sources.  Maybe the 
> modularization initiative has made that worse?
>
> Because RESTARTABLE is already a do ... while, it doesn't need a do 
> while (0) wrapper.
> Extra parens around macro variables wouldn't hurt either.
>
> #define RESTARTABLE(_cmd, _result) do {         \
>     (_result) = (_cmd);                         \
> } while (((_result) == -1) && (errno == EINTR))
>
> It seems worthwhile to do an audit of all the other calls to 
> RESTARTABLE.  Bugs like this often come in flocks, especially given 
> the copy/paste nature of these files.  A quick look suggests that 
> there is a serious bug here:
>
> RESTARTABLE(dup((int)fd), res);
>     if (fd == -1) {
> throwUnixException(env, errno);
>     }
>
>
>
> On Wed, Aug 20, 2014 at 11:59 PM, Florian Weimer <fweimer at redhat.com 
> <mailto:fweimer at redhat.com>> wrote:
>
>     On 08/20/2014 11:24 AM, Ivan Gerasimov wrote:
>
>                         BUGURL:
>                         https://bugs.openjdk.java.net/browse/JDK-8055421
>                         WEBREV:
>                         http://cr.openjdk.java.net/~igerasim/8055421/0/webrev/
>                         <http://cr.openjdk.java.net/%7Eigerasim/8055421/0/webrev/>
>
>
>         It's not clear from the report what are the reason of the observed
>         failure of closedir.
>
>
>     I think the most likely explanation is that closedir succeeded,
>     but errno was -1 for some random reason, and the exception was
>     thrown without without an actual error.  It's still odd that errno
>     was -1, though, but it can obviously happen, it's just some
>     thread-local variable, after all.
>
>
>         However, the bug and fix are quite obvious I think.
>
>
>     Yes, it's clear that the new code is more correct. :-)
>
>
>     -- 
>     Florian Weimer / Red Hat Product Security
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/nio-dev/attachments/20140822/55c7274d/attachment.html>