RFR(XXS): 8242485: Null _file checking in fileStream::flush()

David Holmes david.holmes at oracle.com
Tue Apr 14 07:28:09 UTC 2020


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>
>     Send Time:2020年4月14日(星期二) 13:54
>     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,
> 
>     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>
>      >     Send Time:2020年4月13日(星期一) 06:23
>      >     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()
>      >
>      >     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>
>      >      >     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