Re: RFR(XXS): 8242485: Null _file checking in fileStream::flush()
Denghui Dong
denghui.ddh at alibaba-inc.com
Sun Apr 12 09:01:23 UTC 2020
Hi David,
Thanks for the comments!
> Unfortunately the more I look at this code the more problems I see with
> it. :( We have:
>
> void fileStream::write(const char* s, size_t len) {
> if (_file != NULL) {
> // Make an unused local variable to avoid warning from gcc compiler.
> size_t count = fwrite(s, 1, len, _file);
> }
> update_position(s, len);
> }
>
> but the update_position should be inside the if-statement - no need to
> see a new webrev for that.
update_position is also used in fdStream::write、 bufferedStream::write、and stringStream::write,
and according to the comments in stringStream::write
=====
// Note that the following does not depend on write_len.
// This means that position and count get updated
// even when overflow occurs.
update_position(s, len);
=====
If my understanding is correct, whether or not to call update_position does not depend on the success of the write action.
If there's a problem here, we should also check the usage of update_position in other methods to ensure consistent usage.
Hence, I think we shouldn't put update_position inside the if-statement, at least for the current issue.
------------------------------------------------------------------
From:David Holmes <david.holmes at oracle.com>
Send Time:2020年4月12日(星期日) 15:04
To:董登辉(卓昂) <denghui.ddh at alibaba-inc.com>; hotspot-dev <hotspot-dev at openjdk.java.net>; hotspot-runtime-dev at openjdk.java.net <hotspot-runtime-dev at openjdk.java.net>; Yasumasa Suenaga <suenaga at oss.nttdata.com>; "Liu, Xin" <xxinliu at amazon.com>
Subject:Re: RFR(XXS): 8242485: Null _file checking in fileStream::flush()
Hi Denghui,
On 12/04/2020 2:41 pm, Denghui Dong wrote:
> Hi David,
>
> Thanks for the comments
> new webrev: http://cr.openjdk.java.net/~ddong/8242485/webrev.02
>
> when _file is NULL:
> - fileStream::read return 0
> - fileStream::readln return NULL
> - fileStream::eof return -1 (like
> fileStream::fileSize, not sure if appropriate, feof returns nonzero if target file is end)
That is all great - thanks!
Unfortunately the more I look at this code the more problems I see with
it. :( We have:
void fileStream::write(const char* s, size_t len) {
if (_file != NULL) {
// Make an unused local variable to avoid warning from gcc compiler.
size_t count = fwrite(s, 1, len, _file);
}
update_position(s, len);
}
but the update_position should be inside the if-statement - no need to
see a new webrev for that.
We should also be watching for errors from fwrite - but that in itself
just opens up a can-of-worms with regard to error handling in general
with this class, so that can be left as-is.
Thanks,
David
-----
> Testing: tier1
>
> Cheers,
> Denghui Dong
>
> ------------------------------------------------------------------
> From:David Holmes <david.holmes at oracle.com>
> Send Time:2020年4月12日(星期日) 10:40
> To:董登辉(卓昂) <denghui.ddh at alibaba-inc.com>; hotspot-dev
> <hotspot-dev at openjdk.java.net>; hotspot-runtime-dev at openjdk.java.net
> <hotspot-runtime-dev at openjdk.java.net>; Yasumasa Suenaga
> <suenaga at oss.nttdata.com>; "Liu, Xin" <xxinliu at amazon.com>
> Subject:Re: RFR(XXS): 8242485: Null _file checking in
> fileStream::flush()
>
> Hi,
>
> On 12/04/2020 1:52 am, Denghui Dong wrote:
> > Hi,
> > Thank you for the review and point out other methods need to check _file.
> > In addition to "fileStream::flush", I found there are still four methods need to check _file in fileStream: read, readln, eof, rewind.
> > but those methods will crash if _file is NULL, and I didn't find any caller of them(Maybe I am wrong),
> > so I think it's more appropriate to add assert check to those methods.
>
> Assertions are really only appropriate if they will be exercised by
> tests - the intent being that tests will expose any cases where the
> assertion fails.
>
> Also it doesn't make sense to me to have assertions in most places but a
>
> true NULL check in others - that suggests we expect some methods to be
> called when _file is NULL, but others not. I'd prefer just to see the
> missing NULL checks added, rather than using assertions.
>
> Thanks,
> David
>
> > Webrev: http://cr.openjdk.java.net/~ddong/8242485/webrev.01/
> >
> > Could you review it again, and sponsor it if everything is okay?
> >
> > Testing: teir1
> > ==============================
> > Test summary
> > ==============================
> > TEST TOTAL PASS FAIL ERROR
> > jtreg:test/hotspot/jtreg:tier1 1516 1516 0 0
> > jtreg:test/jdk:tier1 1904 1904 0 0
> > >> jtreg:test/langtools:tier1 4031 4029 2 0 <<
> > jtreg:test/nashorn:tier1 0 0 0 0
> > jtreg:test/jaxp:tier1 0 0 0 0
> > ==============================
> > There are two unrelated test failures in
> > langtools: jdk/javadoc/tool/CheckResourceKeys.java and tools/javac/processing/model/TestSymtabItems.java
> >
> >
> >
> > ------------------------------------------------------------------
> > From:Yasumasa Suenaga <suenaga at oss.nttdata.com>
> > Send Time:2020年4月11日(星期六) 09:22
> > To:董登辉(卓昂) <denghui.ddh at alibaba-inc.com>; hotspot-dev
> > <hotspot-dev at openjdk.java.net>; hotspot-runtime-dev at openjdk.java.net
> > <hotspot-runtime-dev at openjdk.java.net>
> > Subject:Re: RFR(XXS): 8242485: Null _file checking in
> > fileStream::flush()
> >
> > Hi Denghui,
> >
> > null check lacks in other place too.
> > Can you fix it?
> >
> > http://hg.openjdk.java.net/jdk/jdk/file/97d5d0cd1085/src/hotspot/share/utilities/ostream.cpp#l554
> >
> >
> > Thanks,
> >
> > Yasumasa
> >
> >
> > On 2020/04/10 21:34, Denghui Dong wrote:
> > > Hi team,
> > >
> > > Could you please review this small patch?
> > >
> > > JBS: https://bugs.openjdk.java.net/browse/JDK-8242485
> > > webrev: http://cr.openjdk.java.net/~ddong/8242485/webrev.00/
> > >
> > > I found the method "fileStream::flush()" lacks null check before fflush which will cause
> > > all open output streams are flushed by fflush() if _file is NULL.
> > >
> > > Thanks,
> > > Denghui Dong
> > >
> >
>
More information about the hotspot-runtime-dev
mailing list