Re: RFR: 8277204: Implementation of JEP 8264130: PAC-RET protection for Linux/AArch64 [v8]
> PAC is an optional feature in AArch64 8.3 and is compulsory in v9. One > of its uses is to protect against ROP based attacks. This is done by > signing the Link Register whenever it is stored on the stack, and > authenticating the value when it is loaded back from the stack. If an > attacker were to try to change control flow by editing the stack then > the authentication check of the Link Register will fail, causing a > segfault when the function returns. > > On a system with PAC enabled, it is expected that all applications will > be compiled with ROP protection. Fedora 33 and upwards already provide > this. By compiling for ARMv8.0, GCC and LLVM will only use the set of > PAC instructions that exist in the NOP space - on hardware without PAC, > these instructions act as NOPs, allowing backward compatibility for > negligible performance cost (2 NOPs per non-leaf function). > > Hardware is currently limited to the Apple M1 MacBooks. All testing has > been done within a Fedora Docker image. A run of SpecJVM showed no > difference to that of noise - which was surprising. > > The most important part of this patch is simply compiling using branch > protection provided by GCC/LLVM. This protects all C++ code from being > used in ROP attacks, removing all static ROP gadgets from use. > > The remainder of the patch adds ROP protection to runtime generated > code, in both stubs and compiled Java code. Attacks here are much harder > as ROP gadgets must be found dynamically at runtime. If/when AOT > compilation is added to JDK, then all stubs and compiled Java will be > susceptible ROP gadgets being found by static analysis and therefore > potentially as vulnerable as C++ code. > > There are a number of places where the VM changes control flow by > rewriting the stack or otherwise. I’ve done some analysis as to how > these could also be used for attacks (which I didn’t want to post here). > These areas can be protected ensuring the pointers to various stubs and > entry points are stored in memory as signed pointers. These changes are > simple to make (they can be reduced to a type change in common code and > a few addition sign/auth calls in the backend), but there a lot of them > and the total code change is fairly large. I’m happy to provide a few > work in progress patches. > > In order to match the security benefits of the Apple Arm64e ABI across > the whole of JDK, then all the changes mentioned above would be > required. Alan Hayward has updated the pull request incrementally with one additional commit since the last revision: Fix up UseROPProtection flag - Changes: - all: https://git.openjdk.java.net/jdk/pull/6334/files - new: https://git.openjdk.java.net/jdk/pull/6334/files/280abc41..995d8aa3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6334&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6334&range=06-07 Stats: 18 lines in 1 file changed: 10 ins; 5 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/6334.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6334/head:pull/6334 PR: https://git.openjdk.java.net/jdk/pull/6334
Re: RFR: 8277204: Implementation of JEP 8264130: PAC-RET protection for Linux/AArch64 [v7]
On Mon, 22 Nov 2021 17:35:41 GMT, Alan Hayward wrote: >> PAC is an optional feature in AArch64 8.3 and is compulsory in v9. One >> of its uses is to protect against ROP based attacks. This is done by >> signing the Link Register whenever it is stored on the stack, and >> authenticating the value when it is loaded back from the stack. If an >> attacker were to try to change control flow by editing the stack then >> the authentication check of the Link Register will fail, causing a >> segfault when the function returns. >> >> On a system with PAC enabled, it is expected that all applications will >> be compiled with ROP protection. Fedora 33 and upwards already provide >> this. By compiling for ARMv8.0, GCC and LLVM will only use the set of >> PAC instructions that exist in the NOP space - on hardware without PAC, >> these instructions act as NOPs, allowing backward compatibility for >> negligible performance cost (2 NOPs per non-leaf function). >> >> Hardware is currently limited to the Apple M1 MacBooks. All testing has >> been done within a Fedora Docker image. A run of SpecJVM showed no >> difference to that of noise - which was surprising. >> >> The most important part of this patch is simply compiling using branch >> protection provided by GCC/LLVM. This protects all C++ code from being >> used in ROP attacks, removing all static ROP gadgets from use. >> >> The remainder of the patch adds ROP protection to runtime generated >> code, in both stubs and compiled Java code. Attacks here are much harder >> as ROP gadgets must be found dynamically at runtime. If/when AOT >> compilation is added to JDK, then all stubs and compiled Java will be >> susceptible ROP gadgets being found by static analysis and therefore >> potentially as vulnerable as C++ code. >> >> There are a number of places where the VM changes control flow by >> rewriting the stack or otherwise. I’ve done some analysis as to how >> these could also be used for attacks (which I didn’t want to post here). >> These areas can be protected ensuring the pointers to various stubs and >> entry points are stored in memory as signed pointers. These changes are >> simple to make (they can be reduced to a type change in common code and >> a few addition sign/auth calls in the backend), but there a lot of them >> and the total code change is fairly large. I’m happy to provide a few >> work in progress patches. >> >> In order to match the security benefits of the Apple Arm64e ABI across >> the whole of JDK, then all the changes mentioned above would be >> required. > > Alan Hayward has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 13 commits: > > - Merge master > - Merge master > - Rename pauth_authenticate_or_strip_return_address > - Fix windows aarch64 by restoring pauth file split > - Don't keep LR live across restore_live_registers > - Merge master > - Document pauth functions && remove OS split > - Update UseROPProtection description > - Simplify branch protection configure check > - 8264130: PAC-RET protection for Linux/AArch64 > >PAC is an optional feature in AArch64 8.3 and is compulsory in v9. One >of its uses is to protect against ROP based attacks. This is done by >signing the Link Register whenever it is stored on the stack, and >authenticating the value when it is loaded back from the stack. If an >attacker were to try to change control flow by editing the stack then >the authentication check of the Link Register will fail, causing a >segfault when the function returns. > >On a system with PAC enabled, it is expected that all applications will >be compiled with ROP protection. Fedora 33 and upwards already provide >this. By compiling for ARMv8.0, GCC and LLVM will only use the set of >PAC instructions that exist in the NOP space - on hardware without PAC, >these instructions act as NOPs, allowing backward compatibility for >negligible performance cost (2 NOPs per non-leaf function). > >Hardware is currently limited to the Apple M1 MacBooks. All testing has >been done within a Fedora Docker image. A run of SpecJVM showed no >difference to that of noise - which was surprising. > >The most important part of this patch is simply compiling using branch >protection provided by GCC/LLVM. This protects all C++ code from being >used in ROP attacks, removing all static ROP gadgets from use. > >The remainder of the patch adds ROP protection to runtime generated >code, in both stubs and compiled Java code. Attacks here are much harder >as ROP gadgets must be found dynamically at runtime. If/when AOT >compilation is added to JDK, then all stubs and compiled Java will be >susceptible ROP gadgets being found by static analysis and therefore >potentially as vulnerable as C++ code. > >There are a number of places where the VM changes control flow by >
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation
On Thu, 2 Dec 2021 00:09:31 GMT, Sergey Bylokhov wrote: > I have a question related to the custom cacerts which can be added to the > OpenJDK bundle. How do you pass the tests like > test/jdk/sun/security/lib/cacerts/VerifyCACerts.java using that custom jdk > bundle? Probably we can add an additional configuration to that test so it > will check the custom cacerts passed to the build as well? @mrserb So VerifyCACerts is specific to the make/data/cacerts certificates, the README specifically states there that when those are updated VerifyCACerts needs updating. It checks things like fingerprints etc.. If a developer or other provider decide to provide their own cacerts file, then it is up to them to have verified and trust those certificates. They won't run the VerifyCACerts which is specific to the openjdk certs. This is the case at Adoptium for example, which uses the Mozilla trusted CA certs. - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
> Addition of a configure option --with-cacerts-src='user cacerts folder' to > allow developers to specify their own cacerts PEM folder for generation of > the cacerts store using the deterministic openjdk GenerateCacerts tool. > > Signed-off-by: Andrew Leonard Andrew Leonard 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: - 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation Signed-off-by: Andrew Leonard - Merge branch 'master' of https://github.com/openjdk/jdk into cacertssrc - 8278080: Add --with-cacerts-src='user cacerts folder' to enable determinsitic cacerts generation Signed-off-by: Andrew Leonard - 8278080: Add --with-cacerts-src='user cacerts folder' to enable determinsitic cacerts generation Signed-off-by: Andrew Leonard - Changes: - all: https://git.openjdk.java.net/jdk/pull/6647/files - new: https://git.openjdk.java.net/jdk/pull/6647/files/1084c4e1..16c5ca6b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6647&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6647&range=00-01 Stats: 1879 lines in 62 files changed: 1045 ins; 297 del; 537 mod Patch: https://git.openjdk.java.net/jdk/pull/6647.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6647/head:pull/6647 PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Wed, 1 Dec 2021 18:47:41 GMT, Erik Joelsson wrote: >> Andrew Leonard 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: >> >> - 8278080: Add --with-cacerts-src='user cacerts folder' to enable >> deterministic cacerts generation >> >>Signed-off-by: Andrew Leonard >> - Merge branch 'master' of https://github.com/openjdk/jdk into cacertssrc >> - 8278080: Add --with-cacerts-src='user cacerts folder' to enable >> determinsitic cacerts generation >> >>Signed-off-by: Andrew Leonard >> - 8278080: Add --with-cacerts-src='user cacerts folder' to enable >> determinsitic cacerts generation >> >>Signed-off-by: Andrew Leonard > > make/autoconf/jdk-options.m4 line 176: > >> 174: [specify alternative cacerts source folder containing >> certificates])]) >> 175: AC_MSG_CHECKING([for cacerts source]) >> 176: if test "x$with_cacerts_src" == x; then > > Before this if block, please assign an empty value to CACERTS_SRC. Otherwise, > if the user happens to have that variable set in the environment, that value > will get recorded by configure, which is usually not something we want. done - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8277204: Implementation of JEP 8264130: PAC-RET protection for Linux/AArch64 [v7]
On Thu, 2 Dec 2021 09:16:59 GMT, Alan Hayward wrote: > @dholmes-ora > > Fixed flags based on comments on the CSR: Flag updates look good - thanks. - PR: https://git.openjdk.java.net/jdk/pull/6334
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 12:13:03 GMT, Andrew Leonard wrote: >> Addition of a configure option --with-cacerts-src='user cacerts folder' to >> allow developers to specify their own cacerts PEM folder for generation of >> the cacerts store using the deterministic openjdk GenerateCacerts tool. >> >> Signed-off-by: Andrew Leonard > > Andrew Leonard 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: > > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > deterministic cacerts generation > >Signed-off-by: Andrew Leonard > - Merge branch 'master' of https://github.com/openjdk/jdk into cacertssrc > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > determinsitic cacerts generation > >Signed-off-by: Andrew Leonard > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > determinsitic cacerts generation > >Signed-off-by: Andrew Leonard Looks good! - Marked as reviewed by erikj (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 12:13:03 GMT, Andrew Leonard wrote: >> Addition of a configure option --with-cacerts-src='user cacerts folder' to >> allow developers to specify their own cacerts PEM folder for generation of >> the cacerts store using the deterministic openjdk GenerateCacerts tool. >> >> Signed-off-by: Andrew Leonard > > Andrew Leonard 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: > > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > deterministic cacerts generation > >Signed-off-by: Andrew Leonard > - Merge branch 'master' of https://github.com/openjdk/jdk into cacertssrc > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > determinsitic cacerts generation > >Signed-off-by: Andrew Leonard > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > determinsitic cacerts generation > >Signed-off-by: Andrew Leonard I don’t have any major concerns with this change, as long as the default cacerts are still the ones that are in the JDK. As an aside, using Mozilla's root certificates might be fine for TLS certificates, but if you need to support code signing certificates you may run into issues with missing CAs as Mozilla's Root program does not support that use case. Also, by overriding the roots included in the JDK, you are taking on the responsibility (which is significant, in my opinion) of ensuring that those roots are trusted and have not been compromised, revoked, have weak keys, etc. - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 14:29:00 GMT, Sean Mullan wrote: > I don’t have any major concerns with this change, as long as the default > cacerts are still the ones that are in the JDK. As an aside, using Mozilla's > root certificates might be fine for TLS certificates, but if you need to > support code signing certificates you may run into issues with missing CAs as > Mozilla's Root program does not support that use case. Also, by overriding > the roots included in the JDK, you are taking on the responsibility (which is > significant, in my opinion) of ensuring that those roots are trusted and have > not been compromised, revoked, have weak keys, etc. @seanjmullan Thanks Sean, I'll pass your comment on, cheers Andrew - PR: https://git.openjdk.java.net/jdk/pull/6647
Integrated: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation
On Wed, 1 Dec 2021 18:30:06 GMT, Andrew Leonard wrote: > Addition of a configure option --with-cacerts-src='user cacerts folder' to > allow developers to specify their own cacerts PEM folder for generation of > the cacerts store using the deterministic openjdk GenerateCacerts tool. > > Signed-off-by: Andrew Leonard This pull request has now been integrated. Changeset: dc2abc9f Author:Andrew Leonard URL: https://git.openjdk.java.net/jdk/commit/dc2abc9f05c2b7c52aeb242082359c48963f9854 Stats: 22 lines in 3 files changed: 22 ins; 0 del; 0 mod 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 17:33:49 GMT, Magnus Ihse Bursie wrote: >> Andrew Leonard 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: >> >> - 8278080: Add --with-cacerts-src='user cacerts folder' to enable >> deterministic cacerts generation >> >>Signed-off-by: Andrew Leonard >> - Merge branch 'master' of https://github.com/openjdk/jdk into cacertssrc >> - 8278080: Add --with-cacerts-src='user cacerts folder' to enable >> determinsitic cacerts generation >> >>Signed-off-by: Andrew Leonard >> - 8278080: Add --with-cacerts-src='user cacerts folder' to enable >> determinsitic cacerts generation >> >>Signed-off-by: Andrew Leonard > > make/modules/java.base/Gendata.gmk line 76: > >> 74: ifneq ($(CACERTS_SRC), ) >> 75: GENDATA_CACERTS_SRC := $(CACERTS_SRC) >> 76: endif > > Does this even work?! You are reassigning the variable after it has been > used. The := assignment means that it not a macro. I would have expected to see something like: ifneq ($(CACERTS_SRC), ) GENDATA_CACERTS_SRC := $(CACERTS_SRC) else GENDATA_CACERTS_SRC := $(TOPDIR)/make/data/cacerts/ endif at line 63. - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 12:13:03 GMT, Andrew Leonard wrote: >> Addition of a configure option --with-cacerts-src='user cacerts folder' to >> allow developers to specify their own cacerts PEM folder for generation of >> the cacerts store using the deterministic openjdk GenerateCacerts tool. >> >> Signed-off-by: Andrew Leonard > > Andrew Leonard 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: > > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > deterministic cacerts generation > >Signed-off-by: Andrew Leonard > - Merge branch 'master' of https://github.com/openjdk/jdk into cacertssrc > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > determinsitic cacerts generation > >Signed-off-by: Andrew Leonard > - 8278080: Add --with-cacerts-src='user cacerts folder' to enable > determinsitic cacerts generation > >Signed-off-by: Andrew Leonard make/modules/java.base/Gendata.gmk line 76: > 74: ifneq ($(CACERTS_SRC), ) > 75: GENDATA_CACERTS_SRC := $(CACERTS_SRC) > 76: endif Does this even work?! You are reassigning the variable after it has been used. The := assignment means that it not a macro. - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 17:35:36 GMT, Magnus Ihse Bursie wrote: >> make/modules/java.base/Gendata.gmk line 76: >> >>> 74: ifneq ($(CACERTS_SRC), ) >>> 75: GENDATA_CACERTS_SRC := $(CACERTS_SRC) >>> 76: endif >> >> Does this even work?! You are reassigning the variable after it has been >> used. The := assignment means that it not a macro. > > I would have expected to see something like: > > ifneq ($(CACERTS_SRC), ) > GENDATA_CACERTS_SRC := $(CACERTS_SRC) > else > GENDATA_CACERTS_SRC := $(TOPDIR)/make/data/cacerts/ > endif > > at line 63. you make a valid point, but i've tested this numerous times, but let me check again - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 17:46:35 GMT, Andrew Leonard wrote: >> I would have expected to see something like: >> >> ifneq ($(CACERTS_SRC), ) >> GENDATA_CACERTS_SRC := $(CACERTS_SRC) >> else >> GENDATA_CACERTS_SRC := $(TOPDIR)/make/data/cacerts/ >> endif >> >> at line 63. > > you make a valid point, but i've tested this numerous times, but let me check > again my assumption was the recipe gets resolved later - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 17:48:04 GMT, Andrew Leonard wrote: >> you make a valid point, but i've tested this numerous times, but let me >> check again > > my assumption was the recipe gets resolved later this was my understanding: https://www.gnu.org/software/make/manual/html_node/Variables-in-Recipes.html This occurs after make has finished reading all the makefiles and the target is determined to be out of date; so, the recipes for targets which are not rebuilt are never expanded. but i'm going to double check I was checking the resultant cacerts correctly in my tests - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: JDK-8273146: Start of release updates for JDK 19 [v7]
> The time to get JDK 19 underway draws nigh, please review this usual set of > start-of-release updates, including CSRs for the javac and javax.lang.model > updates: > > JDK-8277512: Add SourceVersion.RELEASE_19 > https://bugs.openjdk.java.net/browse/JDK-8277512 > > JDK-8277514: Add source 19 and target 19 to javac > https://bugs.openjdk.java.net/browse/JDK-8277514 > > Clean local tier 1 test runs for langtools, jdk, and hotspot. Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits: - Merge branch 'master' into JDK-8273146 - Merge branch 'master' into JDK-8273146 - Merge branch 'master' into JDK-8273146 - Merge branch 'master' into JDK-8273146 - Update symbol information for JDK 18 b25. - Merge branch 'master' into JDK-8273146 - Merge branch 'master' into JDK-8273146 - Remove SharedSpaces options from VMDeprecatedOptions.java. - Merge branch 'master' into JDK-8273146 - Respond to review feedback. - ... and 2 more: https://git.openjdk.java.net/jdk/compare/8b042d14...b1b4ae2b - Changes: https://git.openjdk.java.net/jdk/pull/6493/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6493&range=06 Stats: 3280 lines in 67 files changed: 3237 ins; 4 del; 39 mod Patch: https://git.openjdk.java.net/jdk/pull/6493.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6493/head:pull/6493 PR: https://git.openjdk.java.net/jdk/pull/6493
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 18:03:50 GMT, Andrew Leonard wrote: >> my assumption was the recipe gets resolved later > > this was my understanding: > https://www.gnu.org/software/make/manual/html_node/Variables-in-Recipes.html > > This occurs after make has finished reading all the makefiles and the target > is determined to be out of date; so, the recipes for targets which are not > rebuilt are never expanded. > > but i'm going to double check I was checking the resultant cacerts correctly > in my tests Oh, I didn't expand the diff far enough to actually see the context correctly when I reviewed this as I would never have imagined the conditional to be placed after the rule. While this will work as so far as using the correct files, incremental builds will not be correct, because the rules are defined in the first pass. I very much agree with Magnus that this conditional belongs around line 63. - PR: https://git.openjdk.java.net/jdk/pull/6647
RFR: 8277069: [REDO] JDK-8276743 Make openjdk build Zip Archive generation "reproducible"
This is basically Andrew's old patch that was backed out, with a single change: I'm using ZipFile instead of ZipInputStream to read in the original zip. The latter is not capable of reading the extended attributes that contain the unix permissions. (Why this is so, is not fully clear to me. What's worse, it's by no means clear from the documentation. We should probably file a follow-up bug to improve the Javadoc for ZipInputStream, warning the users to stay away if they can.) I have also added the possibility to opt-out of reproducible building by an argument to SetupZipArchive. This is used in the closed Oracle part of the build, where we create some temporary zip files that are later discarded, and not part of the deliverables. This saves us some unnecessary overhead. - Commit messages: - Allow user to override reproducible building of zip file - Use ZipFile instead of ZipInputStream, which can handle Unix permissions - 8276743: Make openjdk build Zip Archive generation "reproducible" Changes: https://git.openjdk.java.net/jdk/pull/6673/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6673&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277069 Stats: 262 lines in 4 files changed: 255 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/6673.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6673/head:pull/6673 PR: https://git.openjdk.java.net/jdk/pull/6673
Re: RFR: 8277069: [REDO] JDK-8276743 Make openjdk build Zip Archive generation "reproducible"
On Thu, 2 Dec 2021 14:55:14 GMT, Magnus Ihse Bursie wrote:
> This is basically Andrew's old patch that was backed out, with a single
> change: I'm using ZipFile instead of ZipInputStream to read in the original
> zip. The latter is not capable of reading the extended attributes that
> contain the unix permissions. (Why this is so, is not fully clear to me.
> What's worse, it's by no means clear from the documentation. We should
> probably file a follow-up bug to improve the Javadoc for ZipInputStream,
> warning the users to stay away if they can.)
>
> I have also added the possibility to opt-out of reproducible building by an
> argument to SetupZipArchive. This is used in the closed Oracle part of the
> build, where we create some temporary zip files that are later discarded, and
> not part of the deliverables. This saves us some unnecessary overhead.
I tried to find a way to make GitHub display the difference wrt the original,
backed out, patch, but utterly failed. So here's a simple diff. This is the
only way in which this patch differs from the original.
---
./make/jdk/src/classes/build/tools/makezipreproducible/MakeZipReproducible.java
2021-12-02 19:57:38.0 +0100
+++
../../jdk-ALT/open/make/jdk/src/classes/build/tools/makezipreproducible/MakeZipReproducible.java
2021-12-02 15:11:22.0 +0100
@@ -162,13 +162,8 @@
// Process input zip file and add to sorted entries set
boolean processInputEntries(File inFile) throws IOException {
-try (FileInputStream fis = new FileInputStream(inFile);
- ZipInputStream zis = new ZipInputStream(fis)) {
-ZipEntry entry;
-while ((entry = zis.getNextEntry()) != null) {
-entries.put(entry.getName(), entry);
-}
-}
+ZipFile zipFile = new ZipFile(inFile);
+zipFile.stream().forEach(entry -> entries.put(entry.getName(), entry));
return true;
}
-
PR: https://git.openjdk.java.net/jdk/pull/6673
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 18:46:09 GMT, Erik Joelsson wrote: >> this was my understanding: >> https://www.gnu.org/software/make/manual/html_node/Variables-in-Recipes.html >> >> This occurs after make has finished reading all the makefiles and the target >> is determined to be out of date; so, the recipes for targets which are not >> rebuilt are never expanded. >> >> but i'm going to double check I was checking the resultant cacerts correctly >> in my tests > > Oh, I didn't expand the diff far enough to actually see the context correctly > when I reviewed this as I would never have imagined the conditional to be > placed after the rule. While this will work as so far as using the correct > files, incremental builds will not be correct, because the rules are defined > in the first pass. > > I very much agree with Magnus that this conditional belongs around line 63. yes, thanks, feeling rather stupid here! i'll raise an issue to fix - PR: https://git.openjdk.java.net/jdk/pull/6647
Re: RFR: 8277069: [REDO] JDK-8276743 Make openjdk build Zip Archive generation "reproducible"
On Thu, 2 Dec 2021 14:55:14 GMT, Magnus Ihse Bursie wrote: > This is basically Andrew's old patch that was backed out, with a single > change: I'm using ZipFile instead of ZipInputStream to read in the original > zip. The latter is not capable of reading the extended attributes that > contain the unix permissions. (Why this is so, is not fully clear to me. > What's worse, it's by no means clear from the documentation. We should > probably file a follow-up bug to improve the Javadoc for ZipInputStream, > warning the users to stay away if they can.) > > I have also added the possibility to opt-out of reproducible building by an > argument to SetupZipArchive. This is used in the closed Oracle part of the > build, where we create some temporary zip files that are later discarded, and > not part of the deliverables. This saves us some unnecessary overhead. Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6673
Re: RFR: 8277069: [REDO] JDK-8276743 Make openjdk build Zip Archive generation "reproducible"
On Thu, 2 Dec 2021 14:55:14 GMT, Magnus Ihse Bursie wrote: > This is basically Andrew's old patch that was backed out, with a single > change: I'm using ZipFile instead of ZipInputStream to read in the original > zip. The latter is not capable of reading the extended attributes that > contain the unix permissions. (Why this is so, is not fully clear to me. > What's worse, it's by no means clear from the documentation. We should > probably file a follow-up bug to improve the Javadoc for ZipInputStream, > warning the users to stay away if they can.) > > I have also added the possibility to opt-out of reproducible building by an > argument to SetupZipArchive. This is used in the closed Oracle part of the > build, where we create some temporary zip files that are later discarded, and > not part of the deliverables. This saves us some unnecessary overhead. FWIW, I've opened https://bugs.openjdk.java.net/browse/JDK-8278165 to at least get some documentation on the limitations of ZipInputStream. - PR: https://git.openjdk.java.net/jdk/pull/6673
Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]
On Thu, 2 Dec 2021 19:12:37 GMT, Andrew Leonard wrote: >> Oh, I didn't expand the diff far enough to actually see the context >> correctly when I reviewed this as I would never have imagined the >> conditional to be placed after the rule. While this will work as so far as >> using the correct files, incremental builds will not be correct, because the >> rules are defined in the first pass. >> >> I very much agree with Magnus that this conditional belongs around line 63. > > yes, thanks, feeling rather stupid here! i'll raise an issue to fix @andrew-m-leonard Don't be. Make is a horrible programming language, both syntactically and semantically. It's taken me years to be somewhat comfortable with it, and often I just manage to get it right only by sticking to a few, well-proven and battle-hardened patterns. :) - PR: https://git.openjdk.java.net/jdk/pull/6647
RFR: 8278163: --with-cacerts-src variable resolved after GenerateCacerts recipe setup
PR https://github.com/openjdk/jdk/pull/6647 resolved the GENDATA_CACERTS_SRC fir --with-cacerts-src after the recipe, which meant the dependencies were wrong. This PR moves it before the recipe. Signed-off-by: Andrew Leonard - Commit messages: - 8278163: --with-cacerts-src variable resolved after GenerateCacerts recipe setup Changes: https://git.openjdk.java.net/jdk/pull/6680/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6680&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8278163 Stats: 8 lines in 1 file changed: 4 ins; 3 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6680.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6680/head:pull/6680 PR: https://git.openjdk.java.net/jdk/pull/6680
Integrated: 8277069: [REDO] JDK-8276743 Make openjdk build Zip Archive generation "reproducible"
On Thu, 2 Dec 2021 14:55:14 GMT, Magnus Ihse Bursie wrote: > This is basically Andrew's old patch that was backed out, with a single > change: I'm using ZipFile instead of ZipInputStream to read in the original > zip. The latter is not capable of reading the extended attributes that > contain the unix permissions. (Why this is so, is not fully clear to me. > What's worse, it's by no means clear from the documentation. We should > probably file a follow-up bug to improve the Javadoc for ZipInputStream, > warning the users to stay away if they can.) > > I have also added the possibility to opt-out of reproducible building by an > argument to SetupZipArchive. This is used in the closed Oracle part of the > build, where we create some temporary zip files that are later discarded, and > not part of the deliverables. This saves us some unnecessary overhead. This pull request has now been integrated. Changeset: c93552c8 Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/c93552c8bbcdabb6219327d67409bf63432f49d8 Stats: 262 lines in 4 files changed: 255 ins; 0 del; 7 mod 8277069: [REDO] JDK-8276743 Make openjdk build Zip Archive generation "reproducible" Co-authored-by: Andrew Leonard Co-authored-by: Magnus Ihse Bursie Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/6673
RFR: 8251400: Fix incorrect addition of library to test in JDK-8237858
In JDK-8237858, -pthread was added to all native tests, instead of the one single test that needed it. In the meantime, two new tests with pthread dependencies has crept in unnoticed due to this. - Commit messages: - 8251400: Fix incorrect addition of library to test in JDK-8237858 Changes: https://git.openjdk.java.net/jdk/pull/6682/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6682&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8251400 Stats: 13 lines in 2 files changed: 5 ins; 7 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6682.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6682/head:pull/6682 PR: https://git.openjdk.java.net/jdk/pull/6682
RFR: JDK-8272945: Use snippets in java.compiler documentation
Please review a patch to use snippets in the `java.compiler` documentation,
instead of a mix of raw HTML and/or `{@code ...}`. The change is just about
the presentation of the code fragments; there are no changes to the normative
specifications for the module.
One of the examples went to extraordinary lengths to include the character
sequence `*/` within the example. That example has been replaced by an external
snippet in a separate source file, which does not have any restriction on the
use of `*/`. However, it does require that the file be excluded from standard
compilation, and that is done by setting `EXCLUDES`, once for the "interim"
compiler, and once again for the "product" compiler.Going forward, a better
solution might be to modify the `SetupJavaCompilation` macro to ignore all
directories whose name is not a valid Java identifier (or, if easier, contains
a `-`, such as `doc-files` or `snippet-files`.)
-
Commit messages:
- JDK-8272945: Use snippets in java.compiler documentation
Changes: https://git.openjdk.java.net/jdk/pull/6686/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6686&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8272945
Stats: 123 lines in 7 files changed: 51 ins; 25 del; 47 mod
Patch: https://git.openjdk.java.net/jdk/pull/6686.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/6686/head:pull/6686
PR: https://git.openjdk.java.net/jdk/pull/6686
Re: RFR: JDK-8272945: Use snippets in java.compiler documentation
On Fri, 3 Dec 2021 00:26:10 GMT, Jonathan Gibbons wrote:
> Please review a patch to use snippets in the `java.compiler` documentation,
> instead of a mix of raw HTML and/or `{@code ...}`. The change is just about
> the presentation of the code fragments; there are no changes to the normative
> specifications for the module.
>
> One of the examples went to extraordinary lengths to include the character
> sequence `*/` within the example. That example has been replaced by an
> external snippet in a separate source file, which does not have any
> restriction on the use of `*/`. However, it does require that the file be
> excluded from standard compilation, and that is done by setting `EXCLUDES`,
> once for the "interim" compiler, and once again for the "product" compiler.
> Going forward, a better solution might be to modify the
> `SetupJavaCompilation` macro to ignore all directories whose name is not a
> valid Java identifier (or, if easier, contains a `-`, such as `doc-files` or
> `snippet-files`.)
make/modules/java.compiler/Java.gmk line 30:
> 28:
> 29: EXCLUDES += \
> 30: javax/tools/snippet-files \
You can put this just on a single line :-).
And I'm frankly not sure if make is happy about having a trailing backslash but
no additional line...
src/java.compiler/share/classes/javax/tools/snippet-files/JavaSourceFromString.java
line 29:
> 27: return code;
> 28: }
> 29: }
Missing newline..?
-
PR: https://git.openjdk.java.net/jdk/pull/6686
RFR: JDK-8278175: Enable all doclint warnings for build of java.desktop
In JDK 18, JDK-8189591 added the ability to suppress doclint warnings. Therefore, it is now possible to enable the full doclint checks for the java.desktop module if the instances of warnings are suppressed. This patch does this; it would be preferable to address the doc warnings directly, but that is beyond what I'm attempting to do here. - Commit messages: - JDK-8278175: Enable all doclint warnings for build of java.desktop Changes: https://git.openjdk.java.net/jdk/pull/6687/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6687&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8278175 Stats: 35 lines in 20 files changed: 16 ins; 0 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/6687.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6687/head:pull/6687 PR: https://git.openjdk.java.net/jdk/pull/6687
Re: RFR: JDK-8278175: Enable all doclint warnings for build of java.desktop
On Fri, 3 Dec 2021 01:18:20 GMT, Joe Darcy wrote: > In JDK 18, JDK-8189591 added the ability to suppress doclint warnings. > Therefore, it is now possible to enable the full doclint checks for the > java.desktop module if the instances of warnings are suppressed. This patch > does this; it would be preferable to address the doc warnings directly, but > that is beyond what I'm attempting to do here. Looks OK to me, although I don't consider myself a Reviewer for this code. - PR: https://git.openjdk.java.net/jdk/pull/6687
Re: RFR: 8251400: Fix incorrect addition of library to test in JDK-8237858
On Thu, 2 Dec 2021 22:45:37 GMT, Magnus Ihse Bursie wrote: > In JDK-8237858, -pthread was added to all native tests, instead of the one > single test that needed it. In the meantime, two new tests with pthread > dependencies has crept in unnoticed due to this. Looks good. I don't think `-pthread` would actually hurt anything though. Thanks, David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6682
RFR: JDK-8278179: Enable all doclint warnings for build of java.naming
An exploratory build indicates that it is not necessary to disable the accessibility doclint check for the java.naming module. - Commit messages: - JDK-8278179: Enable all doclint warnings for build of java.naming Changes: https://git.openjdk.java.net/jdk/pull/6688/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6688&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8278179 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6688.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6688/head:pull/6688 PR: https://git.openjdk.java.net/jdk/pull/6688
Re: RFR: JDK-8278179: Enable all doclint warnings for build of java.naming
On Fri, 3 Dec 2021 03:28:46 GMT, Joe Darcy wrote: > An exploratory build indicates that it is not necessary to disable the > accessibility doclint check for the java.naming module. 👍 - Marked as reviewed by iris (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6688
Re: RFR: JDK-8273146: Start of release updates for JDK 19 [v8]
> The time to get JDK 19 underway draws nigh, please review this usual set of > start-of-release updates, including CSRs for the javac and javax.lang.model > updates: > > JDK-8277512: Add SourceVersion.RELEASE_19 > https://bugs.openjdk.java.net/browse/JDK-8277512 > > JDK-8277514: Add source 19 and target 19 to javac > https://bugs.openjdk.java.net/browse/JDK-8277514 > > Clean local tier 1 test runs for langtools, jdk, and hotspot. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Update symbol files to JDK 18 b26. - Changes: - all: https://git.openjdk.java.net/jdk/pull/6493/files - new: https://git.openjdk.java.net/jdk/pull/6493/files/b1b4ae2b..88273596 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6493&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6493&range=06-07 Stats: 610 lines in 3 files changed: 593 ins; 3 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/6493.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6493/head:pull/6493 PR: https://git.openjdk.java.net/jdk/pull/6493
