RFR(XXS): 8242485: Null _file checking in fileStream::flush()
Thomas Stüfe
thomas.stuefe at gmail.com
Wed Apr 15 06:16:59 UTC 2020
On Wed, Apr 15, 2020 at 8:07 AM David Holmes <david.holmes at oracle.com>
wrote:
> 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 :)
>
>
Intention was a polite unsure smile as in "I'm not sure if this is a
problem". Also, I had no coffee yet :)
> 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.
>
Ah okay, good. I really hate those type of errors.
Cheers, Thomas
>
> 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