Re: RFR: 8324657: Intermittent OOME on exception message create
On Mon, 22 Jan 2024 20:52:32 GMT, Roger Riggs wrote:
> When an exception handler for an OutOfMemoryError uses string concatenation
> to compose an exception message, the invoke dynamic string format
> implementation may itself exhaust memory, preventing the exception from being
> handled.
> Explicit use of String.concat() call can improve exception handling.
>
> Writing a test of the exact failure condition has proved challenging due to
> the unpredictable state of memory when OOME occurs. The replacement of "+"
> with String.concat() is simple and direct.
src/java.base/share/classes/java/io/ObjectInputStream.java line 2016:
> 2014: // Generate an InvalidObjectException for an OutOfMemoryError
> 2015: // Use String.concat() to avoid string formatting invoke dynamic
> 2016: private static InvalidObjectException
> genInvalidObjectException(OutOfMemoryError oome, String[] ifaces) {
Isn't this new line is too long? It seems most of the file uses 80 chars per
line.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17522#discussion_r1465700346
Integrated: 8323562: SaslInputStream.read() may return wrong value
On Thu, 11 Jan 2024 06:28:51 GMT, Sergey Bylokhov wrote: > SaslInputStream.read() should return a value in the range from 0 to 255 per > the spec of InputStream.read() but it returns the signed byte from the inBuf > as is. This pull request has now been integrated. Changeset: 5cf7947c Author: Sergey Bylokhov URL: https://git.openjdk.org/jdk/commit/5cf7947ccd1fc56e8944c28145a9c8e71f5e1a03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8323562: SaslInputStream.read() may return wrong value Co-authored-by: Aleksey Shipilev Reviewed-by: shade, dfuchs - PR: https://git.openjdk.org/jdk/pull/17365
Re: RFR: 8323562: SaslInputStream.read() may return wrong value
On Fri, 12 Jan 2024 07:22:09 GMT, Alan Bateman wrote: > Just curious if this was found by inspection or when debugging some issue > with LDAP authentication? Asking on whether it is feasible or not to have > more tests in this area. It was found by the code inspection. - PR Comment: https://git.openjdk.org/jdk/pull/17365#issuecomment-1888567273
RFR: 8323562: SaslInputStream.read() may return wrong value
SaslInputStream.read() should return a value in the range from 0 to 255 per the spec of InputStream.read() but it returns the signed byte from the inBuf as is. - Commit messages: - 8323562: SaslInputStream.read() may return wrong value Changes: https://git.openjdk.org/jdk/pull/17365/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17365=00 Issue: https://bugs.openjdk.org/browse/JDK-8323562 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17365.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17365/head:pull/17365 PR: https://git.openjdk.org/jdk/pull/17365
Re: RFR: 8323562: SaslInputStream.read() may return wrong value
On Thu, 11 Jan 2024 06:28:51 GMT, Sergey Bylokhov wrote: > SaslInputStream.read() should return a value in the range from 0 to 255 per > the spec of InputStream.read() but it returns the signed byte from the inBuf > as is. I'll wait until GA will be fixed. - PR Comment: https://git.openjdk.org/jdk/pull/17365#issuecomment-1888222498
Re: RFR: 8320608: Many jtreg printing tests are missing the @printer keyword [v3]
On Thu, 30 Nov 2023 18:23:28 GMT, Phil Race wrote: >> Many printing tests do not have the @printer keyword. This adds them to >> those that need it. >> I also found one test that has nothing to do with printing in the print >> folder and moved it out. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8320608 Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16785#pullrequestreview-1758403498
Re: RFR: 8320608: Many jtreg printing tests are missing the @printer keyword [v3]
On Thu, 30 Nov 2023 18:23:28 GMT, Phil Race wrote: >> Many printing tests do not have the @printer keyword. This adds them to >> those that need it. >> I also found one test that has nothing to do with printing in the print >> folder and moved it out. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8320608 test/jdk/TEST.ROOT line 19: > 17: # These keywords are there to help with test selection so that > 18: # tests that need a particular resource can be selected to run on a system > 19: # with that resource. Conversely "!somekeyword" can be used to exclude > tests probably we can link this from our wiki page where we describe the usage of keywords?: https://wiki.openjdk.org/display/ClientLibs/Automated+client+GUI+testing+system+set+up+requirements BTW the "doc" has nothing related to printers right now as well: https://github.com/openjdk/jdk/blob/master/doc/testing.md - PR Review Comment: https://git.openjdk.org/jdk/pull/16785#discussion_r1411189538
Re: RFR: 8318364: Add an FFM-based implementation of harfbuzz OpenType layout [v4]
On Tue, 7 Nov 2023 00:37:47 GMT, Phil Race wrote: > > > So we have somewhere around a fixed 125ms startup cost for the FFM case - > > > as measured on my Mac, > > > but only 35-40ms of that is attributable to the specific needs of layout. > > > > > > That looks unfortunate. I guess if we will start to use ffm in other places > > we can easily spend of 1 second budget on startup=( > > Yes, this case is sufficiently uncommon, that it is OK, and is a decent way > to help us track improvements to FFM. But it would be another matter to have > to do it for however many of our core software loops and AWT window manager > interaction calls we need to get running for a minimal app. > > > > layoutCnt=16000 total=193ms <<< app fully displayed > > > vs > > > layoutCnt=16000 total=453ms <<< app fully displayed > > > > > > It looks strange that 16000 calls are not enough to warmup, and the > > difference is so large. > > I am not a C2 expert, (not even an amateur), I just assume that it takes a > lot of calls to be fully optimized. @JornVernee this looks suspicious and seems unrelated to the cold startup issues we discussed before. - PR Comment: https://git.openjdk.org/jdk/pull/15476#issuecomment-1813640596
Re: RFR: 8254693: Add Panama feature to pass heap segments to native code [v14]
On Mon, 13 Nov 2023 12:51:36 GMT, Jorn Vernee wrote: >> Add the ability to pass heap segments to native code. This requires using >> `Linker.Option.critical(true)` as a linker option. It has the same >> limitations as normal critical calls, namely: upcalls into Java are not >> allowed, and the native function should return relatively quickly. Heap >> segments are exposed to native code through temporary native addresses that >> are valid for the duration of the native call. >> >> The motivation for this is supporting existing Java array-based APIs that >> might have to pass multi-megabyte size arrays to native code, and are >> current relying on Get-/ReleasePrimitiveArrayCritical from JNI. Where making >> a copy of the array would be overly prohibitive. >> >> Components of this patch: >> >> - New binding operator `SegmentBase`, which gets the base object of a >> `MemorySegment`. >> - Rename `UnboxAddress` to `SegmentOffset`. Add flag to specify whether >> processing heap segments should be allowed. >> - `CallArranger` impls use new binding operators when >> `Linker.Option.critical(/* allowHeap= */ true)` is specified. >> - `NativeMethodHandle`/`NativeEntryPoint` allow `Object` in their signatures. >> - The object/oop + offset is exposed as temporary address to native code. >> - Since we stay in the `_thread_in_Java` state, we can safely expose the >> oops passed to the downcall stub to native code, without needing GCLocker. >> These oops are valid until we poll for safepoint, which we never do >> (invoking pure native code). >> - Only x64 and AArch64 for now. >> - I've refactored `ArgumentShuffle` in the C++ code to no longer rely on >> callbacks to get the set of source and destination registers (using >> `CallingConventionClosure`), but instead just rely on 2 equal size arrays >> with source and destination registers. This allows filtering the input java >> registers before passing them to `ArgumentShuffle`, which is required to >> filter out registers holding segment offsets. Replacing placeholder >> registers is also done as a separate pre-processing step now. See changes >> in: >> https://github.com/openjdk/jdk/pull/16201/commits/d2b40f1117d63cc6d74e377bf88cdcf6d15ff866 >> - I've factored out `DowncallStubGenerator` in the x64 and AArch64 code to >> use a common `DowncallLinker::StubGenerator`. >> - Fallback linker is also supported using JNI's >> `GetPrimitiveArrayCritical`/`ReleasePrimitiveArrayCritical` >> >> Aside: fixed existing issue with `DowncallLinker` not properly acquiring >> segments in interpreted mode. >> >> Numbers for the included benchmark on my machine are: >> >> >> Benchmar... > > Jorn Vernee has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 52 commits: > > - Merge branch 'master' into AllowHeapNoLock > - fix type and reformat doc in Linker > - Merge branch 'master' into AllowHeapNoLock > - tweak whitespace > - a -> an > - add note to downcallHandle about passing heap segments by-reference > - Merge branch 'master' into AllowHeapNoLock > - bump up argument counts in TestLargeStub to their maximum > - s390 updates > - add stub size stress test for allowHeap > - ... and 42 more: https://git.openjdk.org/jdk/compare/03db8281...36da79d1 Can I assume it will always cause the fatal error, or it is not specified/UB? - PR Comment: https://git.openjdk.org/jdk/pull/16201#issuecomment-1811585932
Re: RFR: 8254693: Add Panama feature to pass heap segments to native code [v14]
On Mon, 13 Nov 2023 12:51:36 GMT, Jorn Vernee wrote: >> Add the ability to pass heap segments to native code. This requires using >> `Linker.Option.critical(true)` as a linker option. It has the same >> limitations as normal critical calls, namely: upcalls into Java are not >> allowed, and the native function should return relatively quickly. Heap >> segments are exposed to native code through temporary native addresses that >> are valid for the duration of the native call. >> >> The motivation for this is supporting existing Java array-based APIs that >> might have to pass multi-megabyte size arrays to native code, and are >> current relying on Get-/ReleasePrimitiveArrayCritical from JNI. Where making >> a copy of the array would be overly prohibitive. >> >> Components of this patch: >> >> - New binding operator `SegmentBase`, which gets the base object of a >> `MemorySegment`. >> - Rename `UnboxAddress` to `SegmentOffset`. Add flag to specify whether >> processing heap segments should be allowed. >> - `CallArranger` impls use new binding operators when >> `Linker.Option.critical(/* allowHeap= */ true)` is specified. >> - `NativeMethodHandle`/`NativeEntryPoint` allow `Object` in their signatures. >> - The object/oop + offset is exposed as temporary address to native code. >> - Since we stay in the `_thread_in_Java` state, we can safely expose the >> oops passed to the downcall stub to native code, without needing GCLocker. >> These oops are valid until we poll for safepoint, which we never do >> (invoking pure native code). >> - Only x64 and AArch64 for now. >> - I've refactored `ArgumentShuffle` in the C++ code to no longer rely on >> callbacks to get the set of source and destination registers (using >> `CallingConventionClosure`), but instead just rely on 2 equal size arrays >> with source and destination registers. This allows filtering the input java >> registers before passing them to `ArgumentShuffle`, which is required to >> filter out registers holding segment offsets. Replacing placeholder >> registers is also done as a separate pre-processing step now. See changes >> in: >> https://github.com/openjdk/jdk/pull/16201/commits/d2b40f1117d63cc6d74e377bf88cdcf6d15ff866 >> - I've factored out `DowncallStubGenerator` in the x64 and AArch64 code to >> use a common `DowncallLinker::StubGenerator`. >> - Fallback linker is also supported using JNI's >> `GetPrimitiveArrayCritical`/`ReleasePrimitiveArrayCritical` >> >> Aside: fixed existing issue with `DowncallLinker` not properly acquiring >> segments in interpreted mode. >> >> Numbers for the included benchmark on my machine are: >> >> >> Benchmar... > > Jorn Vernee has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 52 commits: > > - Merge branch 'master' into AllowHeapNoLock > - fix type and reformat doc in Linker > - Merge branch 'master' into AllowHeapNoLock > - tweak whitespace > - a -> an > - add note to downcallHandle about passing heap segments by-reference > - Merge branch 'master' into AllowHeapNoLock > - bump up argument counts in TestLargeStub to their maximum > - s390 updates > - add stub size stress test for allowHeap > - ... and 42 more: https://git.openjdk.org/jdk/compare/03db8281...36da79d1 Does the usage of this new API can be checked by something like -Xcheck:jni? Especially a part when the app still doing some upcalls to the JVM from native when the pin is "active". - PR Comment: https://git.openjdk.org/jdk/pull/16201#issuecomment-1811531474
Re: RFR: 8318364: Add an FFM-based implementation of harfbuzz OpenType layout [v4]
On Mon, 6 Nov 2023 23:28:30 GMT, Phil Race wrote: >So we have somewhere around a fixed 125ms startup cost for the FFM case - as >measured on my Mac, but only 35-40ms of that is attributable to the specific needs of layout. That looks unfortunate. I guess if we will start to use ffm in other places we can easily spend of 1 second budget on startup=( > layoutCnt=16000 total=193ms <<< app fully displayed vs > layoutCnt=16000 total=453ms <<< app fully displayed It looks strange that 16000 calls are not enough to warmup, and the difference is so large. - PR Comment: https://git.openjdk.org/jdk/pull/15476#issuecomment-1797052585
Re: RFR: 8318364: Add an FFM-based implementation of harfbuzz OpenType layout [v4]
On Wed, 25 Oct 2023 23:42:08 GMT, Phil Race wrote: >> 8318364: Add an FFM-based implementation of harfbuzz OpenType layout > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > indentation Since we plan to import it into jdk22, do you have some performance data to share? any positive or negative effects of this migration? - PR Comment: https://git.openjdk.org/jdk/pull/15476#issuecomment-1795927326
Re: RFR: 8318364: Add an FFM-based implementation of harfbuzz OpenType layout
On Fri, 1 Sep 2023 03:45:24 GMT, Phil Race wrote: > Probably I should not have posted this PR even as draft if it is going to get > attention as it isn't really ready for that. No! That is really interesting proposal and discussion! BTW this PR is not in the draft state. - PR Comment: https://git.openjdk.org/jdk/pull/15476#issuecomment-1702168525
Re: RFR: 8318364: Add an FFM-based implementation of harfbuzz OpenType layout
On Tue, 29 Aug 2023 22:04:40 GMT, Phil Race wrote: > 8318364: Add an FFM-based implementation of harfbuzz OpenType layout @prrace did you check how this change affects the performance, especially startup? I have experimented with Panama for littlecms: https://bugs.openjdk.org/browse/JDK-8313344 and found that the biggest issue is a cold start, 8 ms vs 100ms. An example of the report: https://jmh.morethan.io/?gists=4df0f27789cc4b0ca91fc5b2d677fe39,900b547e073cc1567971f46bfea151db - PR Comment: https://git.openjdk.org/jdk/pull/15476#issuecomment-1698413107
Re: RFR: 8314891: Additional Zip64 extra header validation
On Sat, 9 Sep 2023 14:33:53 GMT, Lance Andersen wrote: > Please review this PR which improves the Zip64 extra header validation: > > - Throw a ZipException If the extra len field is 0 and : > -- size, csize, or loc offset are set to 0x > -- disk starting number is set to 0x > > - We have a valid size for the Zip64 extra header but we are missing the > csize or loc fields if they are expected to be part of the header > > Mach5 tiers 1-3 are clean test/jdk/java/util/zip/ZipFile/MissingZIP64EntriesTest.java line 52: > 50: * starting number is set to 0x or when we have a valid Zip64 Extra > header > 51: * size but missing the corresponding field. > 52: * @run junit MissingZIP64EntriesTest Is this comment accurate? I think we should check 3 cases when the header extra len == 0, len == 8 and len ==16, but still do not contain all required information. - PR Review Comment: https://git.openjdk.org/jdk/pull/15650#discussion_r1322185302
Re: [jdk21] RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size)
On Fri, 18 Aug 2023 17:25:10 GMT, Ben Taylor wrote: > Backport is clean, all tests in test/jdk/java/util/zip pass. It is strange that there is a request for jdk21u but there is no PR for that repo. - PR Comment: https://git.openjdk.org/jdk21/pull/173#issuecomment-1684250803
Re: RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size) [v9]
On Wed, 16 Aug 2023 15:36:53 GMT, Alan Bateman wrote: >As I think has already been said, we can't engage with you in this PR on the >reasons why additional checking was added in a security update. I think you have an assumption that this check for exact size(8/16/24) bytes are related to the change fixed by the security update, I am pretty sure that's the wrong assumption. - PR Comment: https://git.openjdk.org/jdk/pull/15273#issuecomment-1680987371
Re: RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size) [v9]
On Wed, 16 Aug 2023 04:16:52 GMT, Sergey Bylokhov wrote: >> Lance Andersen has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Cleaned up spacing and added missing comma > > My overall point is that it will be unfortunate if users will be able to open > some files on Linux/macOS/Windows using default programs but will not be able > to do that using Java. >@mrserb Have you tested your ZIP file with >-Djdk.util.zip.disableZip64ExtraFieldValidation=true? That's the system >property to disable the additional checking and is the "get out of jail card" >for anyone running into issues. As always with changes like this, or other >changes that tighten up checking, there is a risk that it will break >something, hence the system property to give existing deployments a workaround >to continue. In this case, the original change exposed an issue with a number >of Apache projects (see the linked bugs in their issue trackers) and a bad bug >in the BND tool that was fixed a few years ago. The system property is the >temporary workaround until the deployment has versions of the libraries >produced with updated versions of these tools, or a JDK update that tolerates >a 0 block size. I disagree for a few reasons, using that property will completely disable the appropriate patch for a fix in the CPU, and it will be possible to have/accept some malicious zip files which may trigger some unfortunate behavior. That is not what we would like to recommend doing. Validation of the negative values is much more important. - The bug fixed by the BND was clearly a bug when some "random" value was used as the size of the component which was unrelated to the size of the chunk not the size of the zip file. - The bug we discussed here related to the size of the block which is properly set, for some reason an additional validation was added for it, and it is still not mentioned from where that validation has come, there is no such thing in the spec nor in the behavior of the common tools such as zip/unzip, Windows Explorer, macOS Archive Utility. - PR Comment: https://git.openjdk.org/jdk/pull/15273#issuecomment-1680754080
Re: RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size) [v9]
On Tue, 15 Aug 2023 18:43:36 GMT, Lance Andersen wrote: >> This PR updates the extra field validation added as part of >> [JDK-8302483](https://bugs.openjdk.org/browse/JDK-8302483) to deal with >> issues seen with 3rd party tools/libraries where a ZipException may be >> encountered when opening select APK, ZIP or JAR files. Please see refer to >> the links provided at the end the description for the more information :: >> >>> ZipException: Invalid CEN header (invalid zip64 extra data field size) >> >> 1. Extra field includes padding : >> >> >> #1 >> [Central Directory Header] >> 0x3374: Signature: 0x02014b50 >> 0x3378: Created Zip Spec :0xa [1.0] >> 0x3379: Created OS :0x0 [MS-DOS] >> 0x337a: VerMadeby:0xa [0, 1.0] >> 0x337b: VerExtract :0xa [1.0] >> 0x337c: Flag : 0x800 >> 0x337e: Method :0x0 [STORED] >> 0x3380: Last Mod Time : 0x385ca437 [Thu Feb 28 20:33:46 EST 2008] >> 0x3384: CRC : 0x694c6952 >> 0x3388: Compressed Size : 0x624 >> 0x338c: Uncompressed Size: 0x624 >> 0x3390: Name Length : 0x1b >> 0x3392: Extra Length :0x7 >> [tag=0xcafe, sz=0, data= ] >> ->[tag=cafe, size=0] >> 0x3394: Comment Length :0x0 >> 0x3396: Disk Start :0x0 >> 0x3398: Attrs :0x0 >> 0x339a: AttrsEx :0x0 >> 0x339e: Loc Header Offset:0x0 >> 0x33a2: File Name: res/drawable/size_48x48.jpg >> >> >> The extra field tag of `0xcafe` has its data size set to `0`. and the extra >> length is `7`. It is expected that you can use the tag's data size to >> traverse the extra fields. >> >> >> 2. The [BND tool](https://github.com/bndtools/bnd) added [problematic data >> to the extra field](https://issues.apache.org/jira/browse/FELIX-6622): >> >> >> #359 >> [Central Directory Header] >>0x600b4: Signature: 0x02014b50 >>0x600b8: Created Zip Spec : 0x14 [2.0] >>0x600b9: Created OS :0x0 [MS-DOS] >>0x600ba: VerMadeby: 0x14 [0, 2.0] >>0x600bb: VerExtract : 0x14 [2.0] >>0x600bc: Flag : 0x808 >>0x600be: Method :0x8 [DEFLATED] >>0x600c0: Last Mod Time: 0x2e418983 [Sat Feb 01 17:12:06 EST 2003] >>0x600c4: CRC : 0xd8f689cb >>0x600c8: Compressed Size : 0x23e >>0x600cc: Uncompressed Size: 0x392 >>0x600d0: Name Length : 0x20 >>0x600d2: Extra Length :0x8 >> [tag=0xbfef, sz=61373, data= >> 0x600d4: Comment Length ... > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Cleaned up spacing and added missing comma My overall point is that it will be unfortunate if users will be able to open some files on Linux/macOS/Windows using default programs but will not be able to do that using Java. - PR Comment: https://git.openjdk.org/jdk/pull/15273#issuecomment-1679933388
Re: RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size) [v9]
On Tue, 15 Aug 2023 23:13:00 GMT, Volker Simonis wrote: >As far as I understand you can manually create "artificial" zip files which >can be processed by the zip tool and previous versions of the JDK but not by >new ones. It can be processed by the new/latest version of JDK8. > As long as these kind of files aren't automatically generated by common > tools, I don't see that as a real regression. It is clearly a regression. All that new checks should be proved to be based on some statement from the specification otherwise - such checks should be changed or deleted. As of now the strict check of the size does not based on the spec nor the behavior of the zip cmd. - PR Comment: https://git.openjdk.org/jdk/pull/15273#issuecomment-1679869021
Re: RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size) [v6]
On Tue, 15 Aug 2023 21:38:41 GMT, Volker Simonis wrote: > Did you create that zip file manually or was it created by a tool and if by a > tool than which one? I think we must differentiate here between functional > compatibility with a tool like "zip", compatibility with a specification and > the compatibility with existing zip files and zip files created by common > tools. That was created manually and then repacked by the zip. > The latter is important and required in order to avoid regressions (and I > think that's exactly what we're fixing with this PR). Compatibility with a > specification is great as long as it doesn't collide with the previous point. > Behavioral compatibility with a tool like "zip" is the least important in > this list and I think as long as the file in question is not an artifact > commonly created by popular tools it is fine to behave different for edge > cases. That file is accepted by zip, by the latest JDK8u382, by the JDK20 GA, and rejected by the 20.0.2. That is a regression in the latest update of JDK11+ which we trying to solve here. - PR Comment: https://git.openjdk.org/jdk/pull/15273#issuecomment-1679696095
Re: RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size) [v6]
On Tue, 15 Aug 2023 17:56:09 GMT, Volker Simonis wrote: >This seems to be a very "free" interpretation of the specification to me. >According to my understanding, the valid sizes of 8, 16, 24 or 28 as described >in the Wikipedia article are a direct consequence of the specification. I have provided a test.zip file above which passed the zip integrity test via "zip -T" and can be unzip w/o errors, but rejected by the openjdk. That zip was created based on the actual specification, and not on the wiki. - PR Comment: https://git.openjdk.org/jdk/pull/15273#issuecomment-1679613530
Re: RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size) [v6]
On Tue, 15 Aug 2023 10:49:37 GMT, Alan Bateman wrote: > Are you arguing to drop all checking of the extra fields? It's not clear to > me that this PR should be do that as it has a lot of implications. Not all, but do it in a different way. The only thing which is MUST be implemented according to specifications is: if the data in the body of the zip file for size/csize/locoff is negative then the correct value for these fields should be stored in the extended block. So for example if the size is negative in the body of the zip file, then the extended block should be at least 8 bytes. If the locoff is negative then the extended block should be at least 24 bytes(two fillers at the beginning). Other than that there are no limitation on the size of extended block, it could be 0, 20, 100 , etc. But it should contain correct data if necessary and should not be larger than the surrounding "chunk". - PR Comment: https://git.openjdk.org/jdk/pull/15273#issuecomment-1679198173
Re: RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size) [v6]
On Mon, 14 Aug 2023 21:21:30 GMT, Lance Andersen wrote: >> This PR updates the extra field validation added as part of >> [JDK-8302483](https://bugs.openjdk.org/browse/JDK-8302483) to deal with >> issues seen with 3rd party tools/libraries where a ZipException may be >> encountered when opening select APK, ZIP or JAR files. Please see refer to >> the links provided at the end the description for the more information :: >> >>> ZipException: Invalid CEN header (invalid zip64 extra data field size) >> >> 1. Extra field includes padding : >> >> >> #1 >> [Central Directory Header] >> 0x3374: Signature: 0x02014b50 >> 0x3378: Created Zip Spec :0xa [1.0] >> 0x3379: Created OS :0x0 [MS-DOS] >> 0x337a: VerMadeby:0xa [0, 1.0] >> 0x337b: VerExtract :0xa [1.0] >> 0x337c: Flag : 0x800 >> 0x337e: Method :0x0 [STORED] >> 0x3380: Last Mod Time : 0x385ca437 [Thu Feb 28 20:33:46 EST 2008] >> 0x3384: CRC : 0x694c6952 >> 0x3388: Compressed Size : 0x624 >> 0x338c: Uncompressed Size: 0x624 >> 0x3390: Name Length : 0x1b >> 0x3392: Extra Length :0x7 >> [tag=0xcafe, sz=0, data= ] >> ->[tag=cafe, size=0] >> 0x3394: Comment Length :0x0 >> 0x3396: Disk Start :0x0 >> 0x3398: Attrs :0x0 >> 0x339a: AttrsEx :0x0 >> 0x339e: Loc Header Offset:0x0 >> 0x33a2: File Name: res/drawable/size_48x48.jpg >> >> >> The extra field tag of `0xcafe` has its data size set to `0`. and the extra >> length is `7`. It is expected that you can use the tag's data size to >> traverse the extra fields. >> >> >> 2. The [BND tool](https://github.com/bndtools/bnd) added [problematic data >> to the extra field](https://issues.apache.org/jira/browse/FELIX-6622): >> >> >> #359 >> [Central Directory Header] >>0x600b4: Signature: 0x02014b50 >>0x600b8: Created Zip Spec : 0x14 [2.0] >>0x600b9: Created OS :0x0 [MS-DOS] >>0x600ba: VerMadeby: 0x14 [0, 2.0] >>0x600bb: VerExtract : 0x14 [2.0] >>0x600bc: Flag : 0x808 >>0x600be: Method :0x8 [DEFLATED] >>0x600c0: Last Mod Time: 0x2e418983 [Sat Feb 01 17:12:06 EST 2003] >>0x600c4: CRC : 0xd8f689cb >>0x600c8: Compressed Size : 0x23e >>0x600cc: Uncompressed Size: 0x392 >>0x600d0: Name Length : 0x20 >>0x600d2: Extra Length :0x8 >> [tag=0xbfef, sz=61373, data= >> 0x600d4: Comment Length ... > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Add additional zip to the DataProvider so it is exercised [TEST.zip](https://github.com/openjdk/jdk/files/12340301/TEST.zip) try this example, zip -T passed, unzip works fine, but openjdk rejects it. - PR Comment: https://git.openjdk.org/jdk/pull/15273#issuecomment-1678251452
Re: RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size) [v3]
On Mon, 14 Aug 2023 20:28:08 GMT, Lance Andersen wrote: > > net/n3/nanoxml/CDATAReader.class bad extra-field entry: > > EF block length (61373 bytes) exceeds remaining EF data (4 bytes) > > test of bad.jar FAILED > > zip error: Zip file invalid, could not spawn unzip, or wrong unzip > > (original files unmodified) > > zipdetails would also fail with the above jar It seems that error " EF block length (30837 bytes) exceeds remaining EF data" caused by the fact the size was too big for the actual zipfile, which I think is a different issue, but you can try to unzip that file, and you will get a result w/o errors. unzip implementation is linked above. - PR Comment: https://git.openjdk.org/jdk/pull/15273#issuecomment-1678071099
Re: RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size)
On Mon, 14 Aug 2023 18:41:26 GMT, Sergey Bylokhov wrote: > > I am not understanding your point. There is a specific order for the Zip64 > > fields based on which fields have the Magic value. the spec does also not > > suggest that an empty Zip64 extra field can be written to the CEN when > > there is a Zip64 with data written to the LOC. > > Yes, there is a specific order of fields that should be stored in the > extended block if some of the data in the "body" is negative. But as you > pointed out in this case the empty block or block bigger than necessary to > store the size/csize/locoff is not prohibited by the spec. For example, take > a look at the code in the > [ZipEntry](https://github.com/openjdk/jdk/blob/c132176b932dd136d5c4314e08ac97d0fee7ba4d/src/java.base/share/classes/java/util/zip/ZipEntry.java#L553) > where we accept any size of that block and just checked that it has required > data in it. > > If you disagree then point to the part of the spec which blocks such sizes. This is how it is implemented by the "unzip" https://github.com/madler/zlib/blob/04f42ceca40f73e2978b50e93806c2a18c1281fc/contrib/minizip/unzip.c#L1035C68-L1035C76 , the dataSize is accepted as is. - PR Comment: https://git.openjdk.org/jdk/pull/15273#issuecomment-1677908114
Re: RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size) [v2]
On Mon, 14 Aug 2023 18:22:27 GMT, Lance Andersen wrote:
>> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 3108:
>>
>>> 3106: break;
>>> 3107: }
>>> 3108: if (size == ZIP64_MINVAL) {
>>
>> Note that we always increase "pos" only in case of "_MINVAL". If the values
>> of size and csize are correct/valid in the "body" of the zip file and only
>> locoff is negative then we should skip two fields in the extra block and
>> read the third one. Otherwise, we may read some random values and throw an
>> exception.
>
> I am not sure I (quite) understand your question completely..
>
> How ZIpFS::readExtra has navigated these fields has not changed
>
> If you have a tool that creates a zip/jar that demonstrates an issue that
> might need further examination, please provide a test case, the tool that
> created the zip/jar in question and open a new bug.
The 8302483 changed this code to throw an exception, this is why I am looking
into it.
You can compare the code in this file and the same code in the ZipFile in the
checkZip64ExtraFieldValues method or the code in the ZipEntry#setExtra0, where
we do not increase the "off" but instead checks for "off+8" or "off + 16". So
if we need to read only the third field we should read "pos+16" but for the
current implementation we will read it at "pos+0" since the pos was not bumped
by the code for two other fields.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15273#discussion_r1293844319
Re: RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size)
On Mon, 14 Aug 2023 18:32:15 GMT, Lance Andersen wrote: >I am not understanding your point. There is a specific order for the Zip64 >fields based on which fields have the Magic value. the spec does also not >suggest that an empty Zip64 extra field can be written to the CEN when there >is a Zip64 with data written to the LOC. Yes, there is a specific order of fields that should be stored in the extended block if some of the data in the "body" is negative. But as you pointed out in this case the empty block or block bigger than necessary to store the size/csize/locoff is not prohibited by the spec. For example, take a look at the code in the [ZipEntry](https://github.com/openjdk/jdk/blob/c132176b932dd136d5c4314e08ac97d0fee7ba4d/src/java.base/share/classes/java/util/zip/ZipEntry.java#L553) where we accept any size of that block and just checked that it has required data in it. If you disagree then point to the part of the spec which blocks such sizes. - PR Comment: https://git.openjdk.org/jdk/pull/15273#issuecomment-1677878124
Re: RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size) [v2]
On Mon, 14 Aug 2023 17:33:30 GMT, Lance Andersen wrote:
>> This PR updates the extra field validation added as part of
>> [JDK-8302483](https://bugs.openjdk.org/browse/JDK-8302483) to deal with
>> issues seen with 3rd party tools/libraries where a ZipException may be
>> encountered when opening select APK, ZIP or JAR files. Please see refer to
>> the links provided at the end the description for the more information ::
>>
>>> ZipException: Invalid CEN header (invalid zip64 extra data field size)
>>
>> 1. Extra field includes padding :
>>
>>
>> #1
>> [Central Directory Header]
>> 0x3374: Signature: 0x02014b50
>> 0x3378: Created Zip Spec :0xa [1.0]
>> 0x3379: Created OS :0x0 [MS-DOS]
>> 0x337a: VerMadeby:0xa [0, 1.0]
>> 0x337b: VerExtract :0xa [1.0]
>> 0x337c: Flag : 0x800
>> 0x337e: Method :0x0 [STORED]
>> 0x3380: Last Mod Time : 0x385ca437 [Thu Feb 28 20:33:46 EST 2008]
>> 0x3384: CRC : 0x694c6952
>> 0x3388: Compressed Size : 0x624
>> 0x338c: Uncompressed Size: 0x624
>> 0x3390: Name Length : 0x1b
>> 0x3392: Extra Length :0x7
>> [tag=0xcafe, sz=0, data= ]
>> ->[tag=cafe, size=0]
>> 0x3394: Comment Length :0x0
>> 0x3396: Disk Start :0x0
>> 0x3398: Attrs :0x0
>> 0x339a: AttrsEx :0x0
>> 0x339e: Loc Header Offset:0x0
>> 0x33a2: File Name: res/drawable/size_48x48.jpg
>>
>>
>> The extra field tag of `0xcafe` has its data size set to `0`. and the extra
>> length is `7`. It is expected that you can use the tag's data size to
>> traverse the extra fields.
>>
>>
>> 2. The [BND tool](https://github.com/bndtools/bnd) added [problematic data
>> to the extra field](https://issues.apache.org/jira/browse/FELIX-6622):
>>
>>
>> #359
>> [Central Directory Header]
>>0x600b4: Signature: 0x02014b50
>>0x600b8: Created Zip Spec : 0x14 [2.0]
>>0x600b9: Created OS :0x0 [MS-DOS]
>>0x600ba: VerMadeby: 0x14 [0, 2.0]
>>0x600bb: VerExtract : 0x14 [2.0]
>>0x600bc: Flag : 0x808
>>0x600be: Method :0x8 [DEFLATED]
>>0x600c0: Last Mod Time: 0x2e418983 [Sat Feb 01 17:12:06 EST 2003]
>>0x600c4: CRC : 0xd8f689cb
>>0x600c8: Compressed Size : 0x23e
>>0x600cc: Uncompressed Size: 0x392
>>0x600d0: Name Length : 0x20
>>0x600d2: Extra Length :0x8
>> [tag=0xbfef, sz=61373, data=
>> 0x600d4: Comment Length ...
>
> Lance Andersen has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains three additional
> commits since the last revision:
>
> - Merge branch 'master' into extraHeaders-JDK-8313765
> - Minor comment word smithing
> - Fix for JDK-8313765
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 3108:
> 3106: break;
> 3107: }
> 3108: if (size == ZIP64_MINVAL) {
Note that we always increase "pos" only in case of "_MINVAL". If the values of
size and csize are correct/valid in the "body" of the zip file and only locoff
is negative then we should skip two fields in the extra block and read the
third one. Otherwise, we may read some random values and throw an exception.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15273#discussion_r1293801754
Re: RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size)
On Mon, 14 Aug 2023 17:16:36 GMT, Alan Bateman wrote: > It's unfortunate that there are tools and plugins in the eco system that have > these issues. I think you've got the right balance here, meaning tolerating a > zip64 extra block with a block size of 0 and rejecting corrupted extra blocks > added by older versions of the BND plugin. I think I already asked this question, but it disappeared in the latest PR: Why our code has an assumption that the extended block has some kind of limitation of the size, like 9,16,24,28, there are no such limitations in the zip specification: https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT 4.5.3 -Zip64 Extended Information Extra Field (0x0001): The following is the layout of the zip64 extended information "extra" block. If one of the size or offset fields in the Local or Central directory record is too small to hold the required data, a Zip64 extended information record is created. The order of the fields in the zip64 extended information record is fixed, but the fields MUST only appear if the corresponding Local or Central directory record field is set to 0x or 0x. Note: all fields stored in Intel low-byte/high-byte order. Value Size Description - --- (ZIP64) 0x0001 2 bytesTag for this "extra" block type Size 2 bytesSize of this "extra" block Original Size 8 bytesOriginal uncompressed file size Compressed Size 8 bytesSize of compressed data Relative Header Offset 8 bytesOffset of local header record Disk Start Number 4 bytesNumber of the disk on which this file starts This entry in the Local header MUST include BOTH original and compressed file size fields. If encrypting the It probably comes from the Wiki page: https://en.wikipedia.org/wiki/ZIP_(file_format) but it is not a spec. - PR Comment: https://git.openjdk.org/jdk/pull/15273#issuecomment-1677821187
[jdk21] Integrated: 8313260: JDK21: ProblemList java/lang/ScopedValue/StressStackOverflow.java on linux-x86
On Thu, 27 Jul 2023 18:21:45 GMT, Sergey Bylokhov wrote: > This patch adds the java/lang/ScopedValue/StressStackOverflow.java to the > problem list for linux-x86 where it intermittently fails on a GA, ex: > https://github.com/openjdk/jdk21/pull/148 > > This is only for JDK 21, test passes on JDK 22. This pull request has now been integrated. Changeset: bb63b791 Author:Sergey Bylokhov URL: https://git.openjdk.org/jdk21/commit/bb63b79112852af3020fe64bac1974d26272b442 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8313260: JDK21: ProblemList java/lang/ScopedValue/StressStackOverflow.java on linux-x86 Reviewed-by: clanger - PR: https://git.openjdk.org/jdk21/pull/149
Re: [jdk21] RFR: 8313260: JDK21: ProblemList java/lang/ScopedValue/StressStackOverflow.java on linux-x86
On Thu, 27 Jul 2023 18:21:45 GMT, Sergey Bylokhov wrote: > This patch adds the java/lang/ScopedValue/StressStackOverflow.java to the > problem list for linux-x86 where it intermittently fails on a GA, ex: > https://github.com/openjdk/jdk21/pull/148 > > This is only for JDK 21, test passes on JDK 22. That bug is resolved in JDK 22 and was not backported to JDK21, one of the reasons described here: https://bugs.openjdk.org/browse/JDK-8310822 At the same time, the test was updated in JDK22 by https://bugs.openjdk.org/browse/JDK-8311926. I think one of them solves the problem. - PR Comment: https://git.openjdk.org/jdk21/pull/149#issuecomment-1655112430
[jdk21] RFR: 8313260: JDK21: ProblemList java/lang/ScopedValue/StressStackOverflow.java on linux-x86
This patch adds the java/lang/ScopedValue/StressStackOverflow.java to the problem list for linux-x86 where it intermittently fails on a GA, ex: https://github.com/openjdk/jdk21/pull/148 This is only for JDK 21, test passes on JDK 22. - Commit messages: - use linux-i586 - 8313260: JDK21: ProblemList java/lang/ScopedValue/StressStackOverflow.java on linux-x86 Changes: https://git.openjdk.org/jdk21/pull/149/files Webrev: https://webrevs.openjdk.org/?repo=jdk21=149=00 Issue: https://bugs.openjdk.org/browse/JDK-8313260 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk21/pull/149.diff Fetch: git fetch https://git.openjdk.org/jdk21.git pull/149/head:pull/149 PR: https://git.openjdk.org/jdk21/pull/149
Re: RFR: 8295017: Remove Windows specific workaround in JLI_Snprintf [v5]
On Tue, 25 Jul 2023 23:51:34 GMT, Sergey Bylokhov wrote: >> Julian Waters has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains seven additional >> commits since the last revision: >> >> - Merge branch 'openjdk:master' into patch-1 >> - Merge branch 'openjdk:master' into patch-1 >> - Comment documenting change isn't required >> - Merge branch 'openjdk:master' into patch-1 >> - Comment formatting >> - Remove Windows specific JLI_Snprintf implementation >> - Remove Windows JLI_Snprintf definition > > Thank you! > >>If processing string specifier s, S, or Z, format specification processing >>stops, a NULL is placed at the beginning of the buffer. > > I hope this is not an MS extension/implementation detail since I did not find > this in any other places. >@mrserb this change was to a Windows specific file. That change removed the windows specific version of the JLI_Snprintf, and now we use `#define JLI_Snprintf snprintf` on all platforms. And my question was about that "cross-platform" `snprintf`. As linked in the comment above on Windows it adds the null at the start of the buffer in case of error when a negative value is returned. But is that specified by the c99? - PR Comment: https://git.openjdk.org/jdk/pull/10625#issuecomment-1650963715
Re: RFR: 8295017: Remove Windows specific workaround in JLI_Snprintf [v5]
On Thu, 13 Oct 2022 14:48:29 GMT, Julian Waters wrote: >> The C99 snprintf is available with Visual Studio 2015 and above, alongside >> Windows 10 and the UCRT, and is no longer identical to the outdated Windows >> _snprintf. Since support for the Visual C++ 2017 compiler was removed a >> while ago, we can now safely remove the compatibility workaround on Windows >> and have JLI_Snprintf simply delegate to snprintf. > > Julian Waters has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains seven additional > commits since the last revision: > > - Merge branch 'openjdk:master' into patch-1 > - Merge branch 'openjdk:master' into patch-1 > - Comment documenting change isn't required > - Merge branch 'openjdk:master' into patch-1 > - Comment formatting > - Remove Windows specific JLI_Snprintf implementation > - Remove Windows JLI_Snprintf definition Thank you! >If processing string specifier s, S, or Z, format specification processing >stops, a NULL is placed at the beginning of the buffer. I hope this is not an MS extension/implementation detail since I did not find this in any other places. - PR Comment: https://git.openjdk.org/jdk/pull/10625#issuecomment-1650717499
Re: RFR: 8295017: Remove Windows specific workaround in JLI_Snprintf [v3]
On Thu, 13 Oct 2022 14:34:25 GMT, Julian Waters wrote:
>> Looks good. Thanks.
>
> @dholmes-ora could I trouble you for a sponsor? Thanks!
@TheShermanTanker Working on a similar cleanup, and wonder if is it correct to
assume that the "snprintf" adds "nul" even in case of error.
For example, this code was removed by this patch:
if (rc < 0) {
/* apply ansi semantics */
buffer[size - 1] = '\0';
return (int)size;
} else if (rc == size) {
/* force a null terminator */
buffer[size - 1] = '\0';
}
If the result of "snprintf" was negative we always set the '\0'. But what about
default "snprintf"?
-
PR Comment: https://git.openjdk.org/jdk/pull/10625#issuecomment-1650553077
Re: RFR: 8305113: (tz) Update Timezone Data to 2023c [v3]
On Mon, 3 Apr 2023 23:40:05 GMT, Yoshiki Sato wrote: >> Pleases review this PR. >> The PR includes the following changes. >> - tzdata 2023c >> - Hack code to deal with Cairo's DST end, which is same as done in >> 2014g([JDK-8049343](https://bugs.openjdk.org/browse/JDK-8049343)). To work >> around the the limitation of JSR 310 compiler, where the time 24:00 is >> recognized as 0:00 in the previous day. > > Yoshiki Sato has updated the pull request incrementally with two additional > commits since the last revision: > > - Revert formatted code in ZoneInfoFile.java > - fixing typo again Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13255#pullrequestreview-1371553159
Re: RFR: 8305113: (tz) Update Timezone Data to 2023c
On Fri, 31 Mar 2023 00:02:02 GMT, Yoshiki Sato wrote: > Pleases review this PR. > The PR includes the following changes. > - tzdata 2023c > - Hack code to deal with Cairo's DST end, which is same as done in > 2014g([JDK-8049343](https://bugs.openjdk.org/browse/JDK-8049343)). To work > around the the limitation of JSR 310 compiler, where the time 24:00 is > recognized as 0:00 in the previous day. src/java.base/share/classes/sun/util/calendar/ZoneInfoFile.java line 742: > 740: > 741: return new ZoneInfo(zoneId, rawOffset, dstSavings, checksum, > transitions, > 742: offsets, params, willGMTOffsetChange); do we need to change it here? src/java.base/share/classes/sun/util/resources/TimeZoneNames.java line 2: > 1: /* > 2: * Copyright (c) 1996, 2033, Oracle and/or its affiliates. All rights > reserved. Wrong year. - PR Review Comment: https://git.openjdk.org/jdk/pull/13255#discussion_r1154945016 PR Review Comment: https://git.openjdk.org/jdk/pull/13255#discussion_r1154944389
Integrated: 8300817: The build is broken after JDK-8294693
On Sat, 21 Jan 2023 19:40:58 GMT, Sergey Bylokhov wrote: > The next warning is fixed: > > === Output from failing command(s) repeated here === > * For target jdk_modules_java.base__the.java.base_batch: This pull request has now been integrated. Changeset: 3ea4eac1 Author:Sergey Bylokhov URL: https://git.openjdk.org/jdk/commit/3ea4eac1450954db095ef56385baa3aceea524ea Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8300817: The build is broken after JDK-8294693 Reviewed-by: tvaleev, darcy - PR: https://git.openjdk.org/jdk/pull/12129
RFR: 8300817: The build is broken after JDK-8294693
The next warning is fixed: === Output from failing command(s) repeated here === * For target jdk_modules_java.base__the.java.base_batch: - Commit messages: - The build is broken after JDK-8294693 Changes: https://git.openjdk.org/jdk/pull/12129/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12129=00 Issue: https://bugs.openjdk.org/browse/JDK-8300817 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12129.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12129/head:pull/12129 PR: https://git.openjdk.org/jdk/pull/12129
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v6]
On Sat, 21 Jan 2023 18:31:28 GMT, Tagir F. Valeev wrote: >> Java 17 added RandomGenerator interface. However, existing method >> Collections.shuffle accepts old java.util.Random class. While since Java 19, >> it's possible to use Random.from(RandomGenerator) wrapper, it would be more >> convenient to provide direct overload shuffle(List list, RandomGenerator >> rnd). > > Tagir F. Valeev has updated the pull request incrementally with two > additional commits since the last revision: > > - Whitespaces fixed > - @implSpec added to shuffle(List) Filed, will create a PR soon. https://bugs.openjdk.org/browse/JDK-8300817 - PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v4]
On Wed, 18 Jan 2023 22:06:55 GMT, Tagir F. Valeev wrote: >> Tagir F. Valeev has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains four commits: >> >> - Copyright year and @since tag updated >> - Fixes according to review >> >>1. Reduce duplication in tests >>2. Use JumpableGenerator#copy() instead of create(1) in tests, as >> according to the spec, seed can be ignored >>3. Simplify documentation for shuffle(List, Random) to avoid duplication. >> - Remove Random -> ThreadLocalRandom change >> - 8294693: Add Collections.shuffle overload that accepts RandomGenerator >> interface > > Hm... is something still missing? @amaembo please fix that === Output from failing command(s) repeated here === * For target jdk_modules_java.base__the.java.base_batch: - PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v6]
On Sat, 21 Jan 2023 18:31:28 GMT, Tagir F. Valeev wrote: >> Java 17 added RandomGenerator interface. However, existing method >> Collections.shuffle accepts old java.util.Random class. While since Java 19, >> it's possible to use Random.from(RandomGenerator) wrapper, it would be more >> convenient to provide direct overload shuffle(List list, RandomGenerator >> rnd). > > Tagir F. Valeev has updated the pull request incrementally with two > additional commits since the last revision: > > - Whitespaces fixed > - @implSpec added to shuffle(List) Looks like nobody caught that the build tasks in GActions were broken=( - PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: 8299513: Clean up java.io [v6]
On Tue, 10 Jan 2023 16:06:15 GMT, Per Minborg wrote: >> Code in java.io contains many legacy constructs and semantics not >> recommended including: >> >> * C-style array declaration >> * Unnecessary visibility >> * Redundant keywords in interfaces (e.g. public, static) >> * Non-standard naming for constants >> * Javadoc typos >> * Missing final declaration >> >> These should be fixed as a sanity effort. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Revert removal of static modifier in OSC Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11848
Re: RFR: 8299513: Clean up java.io [v6]
On Tue, 10 Jan 2023 16:06:15 GMT, Per Minborg wrote: >> Code in java.io contains many legacy constructs and semantics not >> recommended including: >> >> * C-style array declaration >> * Unnecessary visibility >> * Redundant keywords in interfaces (e.g. public, static) >> * Non-standard naming for constants >> * Javadoc typos >> * Missing final declaration >> >> These should be fixed as a sanity effort. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Revert removal of static modifier in OSC Please do not type the "/integrate" when none of the reviewer approved the latest meaningful version - PR: https://git.openjdk.org/jdk/pull/11848
Re: RFR: 8299513: Cleanup java.io [v4]
On Mon, 9 Jan 2023 08:57:11 GMT, Per Minborg wrote:
>> Code in java.io contains many legacy constructs and semantics not
>> recommended including:
>>
>> * C-style array declaration
>> * Unnecessary visibility
>> * Redundant keywords in interfaces (e.g. public, static)
>> * Non-standard naming for constants
>> * Javadoc typos
>> * Missing final declaration
>>
>> These should be fixed as a sanity effort.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add additional (c) years
src/java.base/share/classes/java/io/DataInputStream.java line 582:
> 580: * @seejava.io.DataInputStream#readUnsignedShort()
> 581: */
> 582: public static String readUTF(DataInput in) throws IOException {
I remember a few years ago asked to create a CCC to remove the final keyword in
the final class. This change seems broader, probably the rules are changed
since then, but this one actually may affect the method signature. And
subclasses will allow hiding this method.
-
PR: https://git.openjdk.org/jdk/pull/11848
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v2]
On Mon, 9 Jan 2023 08:33:09 GMT, Viktor Klang wrote: >> Currently Set.copyOf allocates both a HashSet and a new empty array when the >> input collection is empty. >> >> This patch avoids allocating anything for the case where the parameter >> collection's isEmpty returns true. > > Viktor Klang has updated the pull request incrementally with two additional > commits since the last revision: > > - 8299444: java.util.Set.copyOf allocates needlessly for empty input > collections > >Modifies ImmutableCollections.listCopy: >Introduces a check for isEmpty to avoid allocation in the case of an > empty input collection. > - 8299444: java.util.Set.copyOf allocates needlessly for empty input > collections > >Modifies Map.copyOf: >Introduces a check for isEmpty to avoid allocation in the case of an empty > input Map. I wonder how it will affect (if any) the performance of the common path, for example if non-empty but small ConcurrentHashMap is passed to the List.copyOf(). - PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections
On Wed, 4 Jan 2023 19:39:23 GMT, Stuart Marks wrote: > so it's difficult to write a simple functional test for this change. It is possible to track that for some "custom" and empty collection the only method will be called is `isEmpty` and nothing else. Not sure how it is useful or not. - PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: JDK-8299052: ViewportOverlapping test fails intermittently on Win10 & Win11 [v2]
On Thu, 22 Dec 2022 18:15:07 GMT, Harshitha Onkar wrote: >> ViewportOverlapping was failing intermittently on Windows (Win10 & 11). >> Added robot.setAutoWaitForIdle() to ViewportOverlapping and its base class >> (OverlappingTestBase) to stabilize the test. >> >> Additionally added awt & swings tests to exclusiveAccess.dir in TEST.ROOT. > > Harshitha Onkar has updated the pull request incrementally with one > additional commit since the last revision: > > reverted TEST.ROOT changes What was the reason for failure? Something was too fast or too slow? - PR: https://git.openjdk.org/jdk/pull/11747
Integrated: 8290899: java/lang/String/StringRepeat.java test requests too much heap on windows x86
On Tue, 13 Dec 2022 00:06:00 GMT, Sergey Bylokhov wrote: > The `java/lang/String/StringRepeat.java` test was updated twice after > integration: > > https://bugs.openjdk.org/browse/JDK-8221400 - the xmx4g was replaced by the > xmx2g > https://bugs.openjdk.org/browse/JDK-8265421 - the "os.maxMemory >= 2G" was > added > > Unfortunately, this test still may fail on Windows x86 due to: `Could not > reserve enough space for xxx object heap.` > > This is a request to exclude it on win x86 since that JDK cannot allocate 2g. This pull request has now been integrated. Changeset: 2bb727c4 Author:Sergey Bylokhov URL: https://git.openjdk.org/jdk/commit/2bb727c4eaf8a948f17f6416a1e6fbaeade4d7ce Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod 8290899: java/lang/String/StringRepeat.java test requests too much heap on windows x86 Reviewed-by: jpai, phh - PR: https://git.openjdk.org/jdk/pull/11639
Re: RFR: 8290899: java/lang/String/StringRepeat.java test requests too much heap on windows x86
On Tue, 13 Dec 2022 00:06:00 GMT, Sergey Bylokhov wrote: > The `java/lang/String/StringRepeat.java` test was updated twice after > integration: > > https://bugs.openjdk.org/browse/JDK-8221400 - the xmx4g was replaced by the > xmx2g > https://bugs.openjdk.org/browse/JDK-8265421 - the "os.maxMemory >= 2G" was > added > > Unfortunately, this test still may fail on Windows x86 due to: `Could not > reserve enough space for xxx object heap.` > > This is a request to exclude it on win x86 since that JDK cannot allocate 2g. If there are no objections or any other comments I'll integrate the fix. - PR: https://git.openjdk.org/jdk/pull/11639
Re: RFR: 8290899: java/lang/String/StringRepeat.java test requests too much heap on windows x86
On Tue, 13 Dec 2022 07:58:36 GMT, Alan Bateman wrote: > What you have looks okay if only Windows 32-bit is skipped but it might be > simpler to just require sun.arch.data.model == "64" so that it only runs on > 64-bit systems. Right, but one of the fix for this test explicitly added support for x86 https://bugs.openjdk.org/browse/JDK-8221400 And on Linux x86 this test works fine. - PR: https://git.openjdk.org/jdk/pull/11639
Re: RFR: 8290899: java/lang/String/StringRepeat.java test requests too much heap on windows x86
On Tue, 13 Dec 2022 04:54:44 GMT, Jaikiran Pai wrote: > Hello Sergey, the linked JBS issues note that this test takes at most 1400M. > Should there be some investigation into why it's failing even with 2GB > allowance? On windows x86 java fails even before the start of the test, it is just impossible to allocate 2g of memory. jdk/bin/java -mx2g --version Error occurred during initialization of VM Could not reserve enough space for 2097152KB object heap The maximum it can allocate is around `-mx1650m`, but using that caused the test to fail from time to time. - PR: https://git.openjdk.org/jdk/pull/11639
RFR: 8290899: java/lang/String/StringRepeat.java test requests too much heap on windows x86
The `java/lang/String/StringRepeat.java` test was updated twice after integration: https://bugs.openjdk.org/browse/JDK-8221400 - the xmx4g was replaced by the xmx2g https://bugs.openjdk.org/browse/JDK-8265421 - the "os.maxMemory >= 2G" was added Unfortunately, this test still may fail on Windows x86 due to: `Could not reserve enough space for xxx object heap.` This is a request to exclude it on win x86 since that JDK cannot allocate 2g. - Commit messages: - 8290899: java/lang/String/StringRepeat.java test requests too much heap on windows x86 Changes: https://git.openjdk.org/jdk/pull/11639/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11639=00 Issue: https://bugs.openjdk.org/browse/JDK-8290899 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11639.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11639/head:pull/11639 PR: https://git.openjdk.org/jdk/pull/11639
Re: RFR: 8298380: Clean up redundant array length checks in JDK code base
On Thu, 8 Dec 2022 12:37:17 GMT, Sergey Tsypanov wrote:
> Newer version of IntelliJ IDEA introduces new
> [inspection](https://youtrack.jetbrains.com/issue/IDEA-301797/IDEA-should-report-redundant-array-length-check-in-certain-cases)
> detecting redundant array length check in snippets like
>
> void iterate(T[] items) {
> if (items.length == 0) {
> return;
> }
> for (T item : items) {
> //...
> }
> }
>
> Here
>
> if (items.length == 0) {
> return;
> }
>
> is redundant and can be removed as length check is performed by for-each loop.
The "client" changes in src/java.desktop looks fine
-
Marked as reviewed by serb (Reviewer).
PR: https://git.openjdk.org/jdk/pull/11589
Re: RFR: 8298047: Remove all non-significant trailing whitespace from properties files
On Fri, 2 Dec 2022 17:06:23 GMT, Magnus Ihse Bursie wrote: > [JDK-8295729](https://bugs.openjdk.org/browse/JDK-8295729) was created in an > attempt to remove all trailing whitespace from properties files, and enable a > jcheck verification that they did not come back, similar to other source > code. It turned out that this was not so simple, however, since trailing > whitespace in values is actually significant in properties files. > > To address this, I have opened four bugs on different component teams to > either remove the trailing significant whitespace (if it is there > erroneously), or convert it to the unicode sequence `\u0020`: > [JDK-8298042](https://bugs.openjdk.org/browse/JDK-8298042), > [JDK-8298044](https://bugs.openjdk.org/browse/JDK-8298044), > [JDK-8298045](https://bugs.openjdk.org/browse/JDK-8298045) and > [JDK-8298046](https://bugs.openjdk.org/browse/JDK-8298046). > > That leaves all the other trailing spaces, in blank lines and in the end of > comments. These serve no purpose and should just be removed, and is what this > issue concerns. > > When this and the four "significant whitespace" bugs are all finally > integrated, we can finally turn on the verification in jcheck for properties > files as well. > > The changes in this PR is based on > [JDK-8295729](https://bugs.openjdk.org/browse/JDK-8295729), in #10792. I have > restored all the "delete trailing whitespace" changes, except for those with > significance. These changes were in turned created by running `find . -type > f -iname "*.properties" | xargs gsed -i -e 's/[ \t]*$//'`. The client changes look fine. - Marked as reviewed by serb (Reviewer). PR: https://git.openjdk.org/jdk/pull/11491
Re: RFR: JDK-8297731: Remove redundant check in MutableBigInteger.divide
On Mon, 28 Nov 2022 19:56:23 GMT, Sergey Bylokhov wrote:
>> src/java.base/share/classes/java/math/MutableBigInteger.java line 1536:
>>
>>> 1534:
>>> 1535: // Must insert leading 0 in rem if its length did not change
>>> 1536: if (rem.intLen == nlen) {
>>
>> Looks like after this change the local nlen variable is used only once, and
>> probably can be removed?
>
> BTW the "rem.leftShift(shift);" above may change the "rem.intLen" value, no?
Sorry I have looked into the different method=), -> divideLongMagnitude.
-
PR: https://git.openjdk.org/jdk/pull/11395
Re: RFR: JDK-8297731: Remove redundant check in MutableBigInteger.divide
On Mon, 28 Nov 2022 19:49:55 GMT, Sergey Bylokhov wrote:
>> Remove redundant code reported in
>>
>> https://mail.openjdk.org/pipermail/core-libs-dev/2022-November/097163.html
>
> src/java.base/share/classes/java/math/MutableBigInteger.java line 1536:
>
>> 1534:
>> 1535: // Must insert leading 0 in rem if its length did not change
>> 1536: if (rem.intLen == nlen) {
>
> Looks like after this change the local nlen variable is used only once, and
> probably can be removed?
BTW the "rem.leftShift(shift);" above may change the "rem.intLen" value, no?
-
PR: https://git.openjdk.org/jdk/pull/11395
Re: RFR: JDK-8297731: Remove redundant check in MutableBigInteger.divide
On Mon, 28 Nov 2022 18:58:29 GMT, Joe Darcy wrote:
> Remove redundant code reported in
>
> https://mail.openjdk.org/pipermail/core-libs-dev/2022-November/097163.html
src/java.base/share/classes/java/math/MutableBigInteger.java line 1536:
> 1534:
> 1535: // Must insert leading 0 in rem if its length did not change
> 1536: if (rem.intLen == nlen) {
Looks like after this change the local nlen variable is used only once, and
probably can be removed?
-
PR: https://git.openjdk.org/jdk/pull/11395
Re: RFR: 8296108: (tz) Update Timezone Data to 2022f
On Wed, 2 Nov 2022 04:10:43 GMT, Yoshiki Sato wrote: > Please review this PR. The PR includes changes in the resource bundle > sources to follow the time zone change observed in America/Chihuahua and > America/Ojinaga. Looks fine - Marked as reviewed by serb (Reviewer). PR: https://git.openjdk.org/jdk/pull/10940
Re: RFR: 8295970: Add jdk_vector tests in GHA [v2]
On Fri, 28 Oct 2022 07:19:31 GMT, Jie Fu wrote: >> Hi all, >> >> As discussed here >> https://github.com/openjdk/jdk/pull/10807#pullrequestreview-1150314487 , it >> would be better to add the vector api tests in GHA. >> >> Thanks. >> Best regards, >> Jie > > Jie Fu has updated the pull request with a new target base due to a merge or > a rebase. The incremental webrev excludes the unrelated changes brought in by > the merge/rebase. The pull request contains four additional commits since the > last revision: > > - Add jdk_vector_sanity test group > - Merge branch 'master' into JDK-8295970 > - Revert changes in test.yml > - 8295970: Add jdk_vector tests in GHA What about possibility to run additional group of the test by passing somehow the name of group to the GA, via label or via /test cmd, or via parameter to the specific task in GA? - PR: https://git.openjdk.org/jdk/pull/10879
