RFR(XXS): 8242485: Null _file checking in fileStream::flush()
David Holmes
david.holmes at oracle.com
Wed Apr 15 06:04:53 UTC 2020
On 15/04/2020 3:56 pm, Thomas Stüfe wrote:
> Will be interesting to see whether we trigger subtle file IO problems
> now, since before we had flushed all streams in the process
> accidentally. We will see whether there was code relying on this
> implicit flush :)
I'm not sure to what extent the smiley is intended to dilute the comment :)
That would only be the case if there was a failure to open a file in the
first place. Further it would require that flush be called on such an
erroneous filestream but not any other method that was failing to do the
null check. So it seems unlikely this would occur and even more unlikely
that the resulting flush-all would be relied upon.
Cheers,
David
> Cheers, Thomas
>
> On Wed, Apr 15, 2020 at 4:12 AM David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
> Changes pushed.
>
> David
>
> On 14/04/2020 6:48 pm, Denghui Dong wrote:
> > http://cr.openjdk.java.net/~ddong/8242485/webrev.02/
> >
> > Added review info.
> >
> > Thanks,
> > <http://cr.openjdk.java.net/~ddong/8242485/webrev.02/>Denghui Dong
> >
> >
> ------------------------------------------------------------------
> > From:David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>>
> > Send Time:2020年4月14日(星期二) 15:28
> > To:董登辉(卓昂) <denghui.ddh at alibaba-inc.com
> <mailto:denghui.ddh at alibaba-inc.com>>; hotspot-dev
> > <hotspot-dev at openjdk.java.net
> <mailto:hotspot-dev at openjdk.java.net>>;
> hotspot-runtime-dev at openjdk.java.net
> <mailto:hotspot-runtime-dev at openjdk.java.net>
> > <hotspot-runtime-dev at openjdk.java.net
> <mailto:hotspot-runtime-dev at openjdk.java.net>>; Yasumasa Suenaga
> > <suenaga at oss.nttdata.com <mailto:suenaga at oss.nttdata.com>>;
> "Liu, Xin" <xxinliu at amazon.com <mailto:xxinliu at amazon.com>>
> > Subject:Re: RFR(XXS): 8242485: Null _file checking in
> > fileStream::flush()
> >
> > On 14/04/2020 4:04 pm, Denghui Dong wrote:
> > > Thanks!
> > >
> > > Could you sponsor it?
> >
> > Sure. Please send me a link to the finalized changeset properly
> > formatted with reviewers etc.
> >
> > Thanks,
> > David
> >
> >
> > ------------------------------------------------------------------
> > > From:David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>>
> > > Send Time:2020年4月14日(星期二) 13:54
> > > To:董登辉(卓昂) <denghui.ddh at alibaba-inc.com
> <mailto:denghui.ddh at alibaba-inc.com>>; hotspot-dev
> > > <hotspot-dev at openjdk.java.net
> <mailto:hotspot-dev at openjdk.java.net>>;
> hotspot-runtime-dev at openjdk.java.net
> <mailto:hotspot-runtime-dev at openjdk.java.net>
> > > <hotspot-runtime-dev at openjdk.java.net
> <mailto:hotspot-runtime-dev at openjdk.java.net>>; Yasumasa Suenaga
> > > <suenaga at oss.nttdata.com
> <mailto:suenaga at oss.nttdata.com>>; "Liu, Xin" <xxinliu at amazon.com
> <mailto:xxinliu at amazon.com>>
> > > Subject:Re: RFR(XXS): 8242485: Null _file checking in
> > > fileStream::flush()
> > >
> > > Hi Denghui,
> > >
> > > This looks fine to me.
> > >
> > > Thanks!
> > >
> > > David
> > >
> > > On 14/04/2020 3:28 pm, Denghui Dong wrote:
> > > > Hi David,
> > > >
> >
> > > After discussing with Yasumasa, I decided to put update_position into
> > > > if-statement,
> >
> > > I also modified fdStream::write, because it obviously has the same problem.
> > > >
> > > > Could you review it again?
> > > > still at
> http://cr.openjdk.java.net/~ddong/8242485/webrev.02
> > > >
> > > > Thanks,
> > > > Denghui Dong
> > > >
> >
> > > ------------------------------------------------------------------
> > > > From:David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>>
> > > > Send Time:2020年4月13日(星期一) 06:23
> > > > To:董登辉(卓昂) <denghui.ddh at alibaba-inc.com
> <mailto:denghui.ddh at alibaba-inc.com>>; hotspot-dev
> > > > <hotspot-dev at openjdk.java.net
> <mailto:hotspot-dev at openjdk.java.net>>;
> hotspot-runtime-dev at openjdk.java.net
> <mailto:hotspot-runtime-dev at openjdk.java.net>
> > > > <hotspot-runtime-dev at openjdk.java.net
> <mailto:hotspot-runtime-dev at openjdk.java.net>>; Yasumasa Suenaga
> > > > <suenaga at oss.nttdata.com
> <mailto:suenaga at oss.nttdata.com>>; "Liu, Xin" <xxinliu at amazon.com
> <mailto:xxinliu at amazon.com>>
> >
> > > Subject:Re: RFR(XXS): 8242485: Null _file checking in
> > > > fileStream::flush()
> > > >
> > > > On 12/04/2020 7:01 pm, Denghui Dong wrote:
> > > > > 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.
> > > >
> >
> > > Every fileStream operation should be a no-op if _file is NULL. Whether
> >
> > > other streams have issues is a different matter. It makes absolutely no
> >
> > > sense to update any state of the filestream in write if nothing is ever
> > > > going to examine it anyway - just a waste of time.
> > > >
> > > > Le's see if there are any other opinions on this.
> > > >
> > > > Thanks,
> > > > David
> > > > -----
> > > >
> >
> > > > 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
> <mailto:david.holmes at oracle.com>>
> > > > > Send Time:2020年4月12日(星期日) 15:04
> > > > > To:董登辉(卓昂) <denghui.ddh at alibaba-
> > inc.com <http://inc.com>>; hotspot-dev
> > > > > <hotspot-dev at openjdk.java.net
> <mailto:hotspot-dev at openjdk.java.net>>;
> hotspot-runtime-dev at openjdk.java.net
> <mailto:hotspot-runtime-dev at openjdk.java.net>
> > > > > <hotspot-runtime-dev at openjdk.java.net
> <mailto:hotspot-runtime-dev at openjdk.java.net>>; Yasumasa Suenaga
> > > > > <suenaga at oss.nttdata.com
> <mailto:suenaga at oss.nttdata.com>>; "Liu, Xin" <xxinliu at amazon.com
> <mailto: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 <mailto:david.holmes at oracle.com>>
> > > > > > Send Time:2020年4月12日(星期日) 10:40
> > > > > > To:董登辉(卓昂) <denghui.ddh at alibaba-
> > > inc.com <http://inc.com>>; hotspot-dev
> > > > > > <hotspot-dev at openjdk.java.net
> <mailto:hotspot-dev at openjdk.java.net>>;
> hotspot-runtime-dev at openjdk.java.net
> <mailto:hotspot-runtime-dev at openjdk.java.net>
> >
> > > > > <hotspot-runtime-dev at openjdk.java.net
> <mailto:hotspot-runtime-dev at openjdk.java.net>>; Yasumasa Suenaga
> > > > > > <suenaga at oss.nttdata.com
> <mailto:suenaga at oss.nttdata.com>>; "Liu, Xin" <xxinliu at amazon.com
> <mailto: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 <mailto:suenaga at oss.nttdata.com>>
> > > > > > > Send Time:2020年4月11日(星
> 期六) 09:22
> > > > > > > To:董登辉(卓
> 昂) <denghui.ddh at alibaba-
> > > > inc.com <http://inc.com>>; hotspot-dev
> >
> > > > > > <hotspot-dev at openjdk.java.net
> <mailto:hotspot-dev at openjdk.java.net>>;
> hotspot-runtime-dev at openjdk.java.net
> <mailto:hotspot-runtime-dev at openjdk.java.net>
> >
> > > > > > <hotspot-runtime-dev at openjdk.java.net <mailto: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