Re: RFR 6722928: Support SSPI as a native GSS-API provider
I've filed https://bugs.openjdk.java.net/browse/JDK-8225687.
> On Jun 11, 2019, at 7:56 AM, Nico Williams wrote:
>
> On Mon, Jun 10, 2019 at 10:30:50AM +0800, Weijun Wang wrote:
>> Updated webrev at
>>
>> http://cr.openjdk.java.net/~weijun/6722928/webrev.08/
>
> -
> src/java.security.jgss/share/classes/sun/security/jgss/spnego/NegTokenTarg.java
>
> Ugh, I never noticed GSSUtil.useMSInterop(). This really should not
> be needed. I believe a proper implementation of RFC4178 wouldn't
> need it.
I tried but scared away by MS-SPNG's appendix on which version of Windows
supports which style of mechListMic. Now that Java only supports one underlying
mechanism this field is looks not so crucial.
>
> I can't expect you to do that, but can we have a bug opened for this?
There was one at https://bugs.openjdk.java.net/browse/JDK-6773898.
>
> -
> src/java.security.jgss/share/classes/sun/security/jgss/wrapper/SunNativeProvider.java
>
> Shouldn't the sspi_bridge.dll be in the lib directory?
The DLLs have always been in /bin. Maybe it's more easily loadable by java.exe.
>
> - src/java.security.jgss/share/native/libj2gss/NativeFunc.h
> - src/java.security.jgss/share/native/libj2gss/gssapi.h
>
> These continue to have improper constification.
>
> There should be no function arguments of type "const gss_...".
>
> I've a PR for Heimdal to fix this in Heimdal. It was very
> mechanical: search the prototypes and function definitions for
>
> \
> then change that to gss_const_... then fix all the warnings and
> errors that came up when building the result.
OK.
>
> - All remaining commentary will be about
> src/java.security.jgss/windows/native/libsspi_bridge/sspi.cpp
>
> - CHECK_*() macros
>
>Macro bodies should not end in a semi-colon.
OK.
>
>If these were public and since these macro bodies are all if
>statements, you should wrap them in do { } while (0), but since
>they're private we can make sure that all uses are correct.
>
> - gss_import_name() doesn't check that the first two bytes of the
>input buffer are the expected token ID when the name-type is
>GSS_C_NT_EXPORTED_NAME.
OK.
>
> - gss_import_name() doesn't check that the third byte of the input
>buffer is 0 when the name-type is GSS_C_NT_EXPORTED_NAME.
OK.
>
> - gss_compare_name(), this code will not distinguish a name of the
>form 'foo@' from 'foo\@'
>
> 434 if (l1 < l2 && n2[l1] != L'@'
> 435 || l2 < l1 && n1[l2] != L'@') {
> 436 return GSS_S_COMPLETE; // different
> 437 }
>
>Honestly, this is not the most serious problem because nothing
>really should be using gss_compare_name(), but you do use it... and
>anyways, it's wrong.
Yes, it's called in GSSLibStub.c and those are the functions I must support.
>
>Perhaps the gss_name_struct struct should have a length of realm or
>length-of-not-realm-part field to make this check easier.
>
> - gss_compare_name(), do not use NORM_IGNORECASE
But I do notice that "Administrator" can also be named "administrator", and
"HTTP/host" can also be "http/host".
>
> - gss_canonicalize_name() should check that mech_type is Kerberos
OK.
>
> I'll continue later. I'm in gss_init_sec_context(), about 58% of the
> way through.
Please.
Thanks,
Max
>
> Nico
> --
Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file
Hi Sean, The previous fix for JDK-8193255 already made the creation date useless. Before that, each time cacerts was regenerated the date changed. I compared cacerts of different releases and the same cert have difference creation dates. The only other solution I can think of is to look at make/autoconf/version-numbers and use the DEFAULT_VERSION_DATE=2019-09-17 there. Have you reviewed the code changes? You can see I stored the hash of the whole file into the test. This means anyone updating the CA certs will have to create cacerts and calculate the correct hash and update this test. I suppose this won't be a hassle. Thanks, Max > On Jun 13, 2019, at 4:15 AM, Sean Mullan wrote: > > On 6/12/19 4:01 PM, Erik Joelsson wrote: >> Hello, >> We cannot rely on querying mercurial at build time. The source must be >> buildable from a source distribution. > > I had a feeling it wouldn't work but thought I would ask anyway. > > Well, offhand I can't think of any better solution than notBefore then, > unless we included it as a comment in the PEM file, and then extracted it. > With notBefore, someone might be curious about why some keystore entries were > created so long ago (ex: the earliest notBefore date is 1996). But the > creationDate doesn't really have any usefulness for cacerts, so it's probably > ok. > > --Sean > >> /Erik >> On 2019-06-12 11:39, Sean Mullan wrote: >>> Using the certificate's notBefore date as the KeyStore entry creation date >>> is misleading since many of these root certs were not integrated into the >>> JDK until after they were created by the CA. Can we somehow extract the >>> last revision time of each PEM file instead? That is more aligned with the >>> previous creation date that we used. >>> >>> --Sean >>> >>> On 6/12/19 12:38 PM, Erik Joelsson wrote: Hello Max, Much appreciated! I will need to have this fixed one way or other in JDK 13, so depending on if you get your fix there in time, I will retract my proposal. If your fix only hits 14, I will push mine to 13. /Erik On 2019-06-12 08:41, Weijun Wang wrote: > This is my version of the fix: > > http://cr.openjdk.java.net/~weijun/8225392/webrev.00/ > > Now you can still compare cacerts bit by bit. > > Thanks, > Max > >> On Jun 12, 2019, at 10:50 PM, Weijun Wang wrote: >> >> Hi Erik, >> >> Are you going to fix this bug soon? >> >> I am inspired by Martin's words and would like to update >> GenerateCacerts.java so that as long as the certs and their aliases are >> unchanged, the output cacerts will always be the same. I can send out a >> code review today. >> >> Thanks, >> Max >> >>> On Jun 12, 2019, at 10:59 AM, Weijun Wang >>> wrote: >>> >>> Good idea about the creation time. >>> >>> --Max >>> On Jun 12, 2019, at 10:53 AM, Martin Buchholz wrote: Google culture really likes build output determinism, and we recently built our own cacerts generator. To get determinism, we are using cert digest as alias (must have a unique alias, but value doesn't seem to matter much), and using cert notBefore instead of current (build) timestamp. On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson wrote: Since JDK-8193255, when we started generating the cacerts file in the build, the build compare baseline builds have started failing. It seems the cacerts binary file has some non determinism built in so it doesn't get generated exactly the same given the same input. This patch adds special handling when comparing that file by comparing the output of "keytool -list" on the files instead. Bug: https://bugs.openjdk.java.net/browse/JDK-8225392 Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/ /Erik
Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file
> On Jun 13, 2019, at 12:16 AM, Martin Buchholz wrote: > > I'm not a security engineer, but: > - consider creating static finals for e.g. "Mighty Aphrodite" just to give it > a symbolic name. That method is copied from JavaKeyStore.java. Keeping it 100% unchanged gives me more confidence I'm write the file correctly. > - VerifyCACerts probably fails when the jdk is configured with a different > cacerts file (but the JDK doesn't preserve configuration information - how > could one fix it?) > Many downstream organizations will configure a different cacerts. I don't have a solution. Maybe in your repo you can @ignore this test. Thanks, Max > > On Wed, Jun 12, 2019 at 8:42 AM Weijun Wang wrote: > This is my version of the fix: > >http://cr.openjdk.java.net/~weijun/8225392/webrev.00/ > > Now you can still compare cacerts bit by bit. > > Thanks, > Max > > > On Jun 12, 2019, at 10:50 PM, Weijun Wang wrote: > > > > Hi Erik, > > > > Are you going to fix this bug soon? > > > > I am inspired by Martin's words and would like to update > > GenerateCacerts.java so that as long as the certs and their aliases are > > unchanged, the output cacerts will always be the same. I can send out a > > code review today. > > > > Thanks, > > Max > > > >> On Jun 12, 2019, at 10:59 AM, Weijun Wang wrote: > >> > >> Good idea about the creation time. > >> > >> --Max > >> > >>> On Jun 12, 2019, at 10:53 AM, Martin Buchholz wrote: > >>> > >>> Google culture really likes build output determinism, and we recently > >>> built our own cacerts generator. > >>> > >>> To get determinism, we are using cert digest as alias (must have a unique > >>> alias, but value doesn't seem to matter much), and using cert notBefore > >>> instead of current (build) timestamp. > >>> > >>> On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson > >>> wrote: > >>> Since JDK-8193255, when we started generating the cacerts file in the > >>> build, the build compare baseline builds have started failing. It seems > >>> the cacerts binary file has some non determinism built in so it doesn't > >>> get generated exactly the same given the same input. This patch adds > >>> special handling when comparing that file by comparing the output of > >>> "keytool -list" on the files instead. > >>> > >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8225392 > >>> > >>> Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/ > >>> > >>> /Erik > >>> > >> > > >
Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file
I plan to fix it in JDK 13. --Max > On Jun 13, 2019, at 12:38 AM, Erik Joelsson wrote: > > Hello Max, > > Much appreciated! I will need to have this fixed one way or other in JDK 13, > so depending on if you get your fix there in time, I will retract my > proposal. If your fix only hits 14, I will push mine to 13. > > /Erik > > On 2019-06-12 08:41, Weijun Wang wrote: >> This is my version of the fix: >> >>http://cr.openjdk.java.net/~weijun/8225392/webrev.00/ >> >> Now you can still compare cacerts bit by bit. >> >> Thanks, >> Max
Re: RFR: 8224851: AArch64: fix warnings and errors with Clang and GCC 8.3
> On Jun 12, 2019, at 5:35 AM, Nick Gasson wrote:
>
> Hi Andrew,
>
> Sorry for the delay, I've put an updated webrev here:
>
> http://cr.openjdk.java.net/~ngasson/8224851/webrev.1/
Since I've been commenting on some parts, I decided I might as well
look at all of it. So here's a review. I don't need to re-review any
of the suggested changes.
--
src/hotspot/cpu/aarch64/frame_aarch64.cpp
772 reg_map = (RegisterMap*)os::malloc(sizeof(RegisterMap), mtNone);
Use NEW_C_HEAP_OBJ(RegisterMap, mtNone)
--
src/hotspot/cpu/aarch64/macroAssembler_aarch64_trig.cpp
384 fcmpd(v26, 0.0); // if NE then jx == 2.
else it's 1 or 0
and
388 fcmpd(v7, 0.0); // v7 == 0 => jx = 0.
Else jx = 1
EOL comments no longer aligned with others nearby.
--
src/hotspot/cpu/aarch64/vm_version_aarch64.cpp
180 if ((p = strchr(buf, ':'))) {
Hotspot Coding Style says not to use implicit bool, instead using
explicit comparisons, e.g. this should be
180 if ((p = strchr(buf, ':')) != NULL) {
That would have avoided the warning in the first place.
And yes, I know there are lots of violations of that guideline.
--
src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp
43 #ifdef __clang__
44 D res = __atomic_add_fetch(dest, add_value, __ATOMIC_RELEASE);
45 FULL_MEM_BARRIER;
46 return res;
47 #else
Is there a reason to conditionalize use of __atomic_add_fetch for
clang and continue to use __sync_add_and_fetch for gcc, rather than
using __atomic_add_fetch unconditionally?
Is __ATOMIC_RELEASE and a following FULL_MEM_BARRIER the right usage
to get the equivalent of __sync_add_and_fetch? Or should
__ATOMIC_SEQ_CST be used? Or should the latter be actively avoided?
I remember some discussion and questions in that area, about how to
implement memory_order_seq_cst on ARM processors, but wasn't paying
close attention since I don't deal with ARM much.
Andrew? I'll happily defer to you here.
--
src/hotspot/os_cpu/linux_aarch64/os_linux_aarch64.cpp
I didn't review the stuff around os::current_stack_pointer and
os::current_frame.
--
src/hotspot/os_cpu/linux_aarch64/copy_linux_aarch64.s
162 prfum pldl1keep, [s, #-256]
I didn't review this change.
--
Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file
Hello, We cannot rely on querying mercurial at build time. The source must be buildable from a source distribution. /Erik On 2019-06-12 11:39, Sean Mullan wrote: Using the certificate's notBefore date as the KeyStore entry creation date is misleading since many of these root certs were not integrated into the JDK until after they were created by the CA. Can we somehow extract the last revision time of each PEM file instead? That is more aligned with the previous creation date that we used. --Sean On 6/12/19 12:38 PM, Erik Joelsson wrote: Hello Max, Much appreciated! I will need to have this fixed one way or other in JDK 13, so depending on if you get your fix there in time, I will retract my proposal. If your fix only hits 14, I will push mine to 13. /Erik On 2019-06-12 08:41, Weijun Wang wrote: This is my version of the fix: http://cr.openjdk.java.net/~weijun/8225392/webrev.00/ Now you can still compare cacerts bit by bit. Thanks, Max On Jun 12, 2019, at 10:50 PM, Weijun Wang wrote: Hi Erik, Are you going to fix this bug soon? I am inspired by Martin's words and would like to update GenerateCacerts.java so that as long as the certs and their aliases are unchanged, the output cacerts will always be the same. I can send out a code review today. Thanks, Max On Jun 12, 2019, at 10:59 AM, Weijun Wang wrote: Good idea about the creation time. --Max On Jun 12, 2019, at 10:53 AM, Martin Buchholz wrote: Google culture really likes build output determinism, and we recently built our own cacerts generator. To get determinism, we are using cert digest as alias (must have a unique alias, but value doesn't seem to matter much), and using cert notBefore instead of current (build) timestamp. On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson wrote: Since JDK-8193255, when we started generating the cacerts file in the build, the build compare baseline builds have started failing. It seems the cacerts binary file has some non determinism built in so it doesn't get generated exactly the same given the same input. This patch adds special handling when comparing that file by comparing the output of "keytool -list" on the files instead. Bug: https://bugs.openjdk.java.net/browse/JDK-8225392 Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/ /Erik
RFR: [8u] JDK-8223219: Backport of JDK-8199552 to OpenJDK 8 leads to duplicate -fstack-protector flags, overriding --with-extra-cflags
Webrev: https://cr.openjdk.java.net/~andrew/openjdk8/8223219/webrev.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8223219 There's quite a long story to this one, more detail of which is in the bug report. In short, JDK-8199552 was backported as part of a CPU with no review and so little explanation for the changes contained therein, which differ from the same change in OpenJDK 11. The end result is that -fstack-protector is added for only two architectures - x86 and x86_64 - and it ends up appearing twice for the JDK part of the build, the second appearance overriding any options specified using --with-extra-cflags. This is a problem for distros which may want to use -fstack-protector-strong instead. This patch simplifies it down to one addition for all architectures. We've been using this patch (or a variant of it) since the January CPU on all architectures we build on (AArch64, s390, s390x, ppc, ppc64, ppc64le, x86 & x86_64) without known issues. Before patch: [7] CFLAGS := -Wall -Wno-parentheses -Wextra -Wno-unused -Wno-unused-parameter -Wformat=2 -pipe -D_GNU_SOURCE -D_REENTRANT -D_LARGEFILE64_SOURCE -fno-omit-frame-pointer -fstack-protector -D_LP64=1 -D_LITTLE_ENDIAN -DLINUX -DARCH='"amd64"' -Damd64 -DNDEBUG -DRELEASE='"1.8.0-internal"' -I/home/andrew/builder/8u-dev/jdk/include -I/home/andrew/builder/8u-dev/jdk/include/linux -I/home/andrew/projects/openjdk/upstream/jdk8u-dev/jdk/src/share/javavm/export -I/home/andrew/projects/openjdk/upstream/jdk8u-dev/jdk/src/solaris/javavm/export -I/home/andrew/projects/openjdk/upstream/jdk8u-dev/jdk/src/share/native/common -I/home/andrew/projects/openjdk/upstream/jdk8u-dev/jdk/src/solaris/native/common -O2 -pipe -march=core2 -mno-tls-direct-seg-refs -Wno-error=return-type -Wno-error=deprecated-declarations -fno-strict-aliasing -fstack-protector -fno-delete-null-pointer-checks -fno-lifetime-dse -fPIC -I/home/andrew/builder/8u-dev/jdk/gensrc_headers -I/home/andrew/projects/openjdk/upstream/jdk8u-dev/jdk/src/share/nativ\ e/java/lang/fdlibm/include After patch (the -fstack-protector duplicate after -fno-strict-aliasing is gone): [7] CFLAGS := -Wall -Wno-parentheses -Wextra -Wno-unused -Wno-unused-parameter -Wformat=2 -pipe -fstack-protector -D_GNU_SOURCE -D_REENTRANT -D_LARGEFILE64_SOURCE -fno-omit-frame-pointer -D_LP64=1 -D_LITTLE_ENDIAN -DLINUX -DARCH='"amd64"' -Damd64 -DNDEBUG -DRELEASE='"1.8.0-internal"' -I/home/andrew/builder/8u-dev/jdk/include -I/home/andrew/builder/8u-dev/jdk/include/linux -I/home/andrew/projects/openjdk/upstream/jdk8u-dev/jdk/src/share/javavm/export -I/home/andrew/projects/openjdk/upstream/jdk8u-dev/jdk/src/solaris/javavm/export -I/home/andrew/projects/openjdk/upstream/jdk8u-dev/jdk/src/share/native/common -I/home/andrew/proje\ cts/openjdk/upstream/jdk8u-dev/jdk/src/solaris/native/common -O2 -pipe -march=core2 -ggdb -mno-tls-direct-seg-refs -Wno-error=return-type -Wno-error=deprecated-declarations -fno-strict-aliasing -fno-delete-null-pointer-checks -fno-lifetime-dse -fPIC -I/home/andrew/builder/8u-dev/jdk/gensrc_headers -I/home/andrew/projects/openjdk/upstream/jdk8u-dev/jdk/src/share/native/java/lang/fdlibm/include Ok to push? Thanks, -- Andrew :) Senior Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net) Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222 https://keybase.io/gnu_andrew
Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file
Using the certificate's notBefore date as the KeyStore entry creation date is misleading since many of these root certs were not integrated into the JDK until after they were created by the CA. Can we somehow extract the last revision time of each PEM file instead? That is more aligned with the previous creation date that we used. --Sean On 6/12/19 12:38 PM, Erik Joelsson wrote: Hello Max, Much appreciated! I will need to have this fixed one way or other in JDK 13, so depending on if you get your fix there in time, I will retract my proposal. If your fix only hits 14, I will push mine to 13. /Erik On 2019-06-12 08:41, Weijun Wang wrote: This is my version of the fix: http://cr.openjdk.java.net/~weijun/8225392/webrev.00/ Now you can still compare cacerts bit by bit. Thanks, Max On Jun 12, 2019, at 10:50 PM, Weijun Wang wrote: Hi Erik, Are you going to fix this bug soon? I am inspired by Martin's words and would like to update GenerateCacerts.java so that as long as the certs and their aliases are unchanged, the output cacerts will always be the same. I can send out a code review today. Thanks, Max On Jun 12, 2019, at 10:59 AM, Weijun Wang wrote: Good idea about the creation time. --Max On Jun 12, 2019, at 10:53 AM, Martin Buchholz wrote: Google culture really likes build output determinism, and we recently built our own cacerts generator. To get determinism, we are using cert digest as alias (must have a unique alias, but value doesn't seem to matter much), and using cert notBefore instead of current (build) timestamp. On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson wrote: Since JDK-8193255, when we started generating the cacerts file in the build, the build compare baseline builds have started failing. It seems the cacerts binary file has some non determinism built in so it doesn't get generated exactly the same given the same input. This patch adds special handling when comparing that file by comparing the output of "keytool -list" on the files instead. Bug: https://bugs.openjdk.java.net/browse/JDK-8225392 Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/ /Erik
Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file
Hello Max, Much appreciated! I will need to have this fixed one way or other in JDK 13, so depending on if you get your fix there in time, I will retract my proposal. If your fix only hits 14, I will push mine to 13. /Erik On 2019-06-12 08:41, Weijun Wang wrote: This is my version of the fix: http://cr.openjdk.java.net/~weijun/8225392/webrev.00/ Now you can still compare cacerts bit by bit. Thanks, Max On Jun 12, 2019, at 10:50 PM, Weijun Wang wrote: Hi Erik, Are you going to fix this bug soon? I am inspired by Martin's words and would like to update GenerateCacerts.java so that as long as the certs and their aliases are unchanged, the output cacerts will always be the same. I can send out a code review today. Thanks, Max On Jun 12, 2019, at 10:59 AM, Weijun Wang wrote: Good idea about the creation time. --Max On Jun 12, 2019, at 10:53 AM, Martin Buchholz wrote: Google culture really likes build output determinism, and we recently built our own cacerts generator. To get determinism, we are using cert digest as alias (must have a unique alias, but value doesn't seem to matter much), and using cert notBefore instead of current (build) timestamp. On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson wrote: Since JDK-8193255, when we started generating the cacerts file in the build, the build compare baseline builds have started failing. It seems the cacerts binary file has some non determinism built in so it doesn't get generated exactly the same given the same input. This patch adds special handling when comparing that file by comparing the output of "keytool -list" on the files instead. Bug: https://bugs.openjdk.java.net/browse/JDK-8225392 Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/ /Erik
Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file
I'm not a security engineer, but: - consider creating static finals for e.g. "Mighty Aphrodite" just to give it a symbolic name. - VerifyCACerts probably fails when the jdk is configured with a different cacerts file (but the JDK doesn't preserve configuration information - how could one fix it?) Many downstream organizations will configure a different cacerts. On Wed, Jun 12, 2019 at 8:42 AM Weijun Wang wrote: > This is my version of the fix: > >http://cr.openjdk.java.net/~weijun/8225392/webrev.00/ > > Now you can still compare cacerts bit by bit. > > Thanks, > Max > > > On Jun 12, 2019, at 10:50 PM, Weijun Wang > wrote: > > > > Hi Erik, > > > > Are you going to fix this bug soon? > > > > I am inspired by Martin's words and would like to update > GenerateCacerts.java so that as long as the certs and their aliases are > unchanged, the output cacerts will always be the same. I can send out a > code review today. > > > > Thanks, > > Max > > > >> On Jun 12, 2019, at 10:59 AM, Weijun Wang > wrote: > >> > >> Good idea about the creation time. > >> > >> --Max > >> > >>> On Jun 12, 2019, at 10:53 AM, Martin Buchholz > wrote: > >>> > >>> Google culture really likes build output determinism, and we recently > built our own cacerts generator. > >>> > >>> To get determinism, we are using cert digest as alias (must have a > unique alias, but value doesn't seem to matter much), and using cert > notBefore instead of current (build) timestamp. > >>> > >>> On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson < > erik.joels...@oracle.com> wrote: > >>> Since JDK-8193255, when we started generating the cacerts file in the > >>> build, the build compare baseline builds have started failing. It > seems > >>> the cacerts binary file has some non determinism built in so it > doesn't > >>> get generated exactly the same given the same input. This patch adds > >>> special handling when comparing that file by comparing the output of > >>> "keytool -list" on the files instead. > >>> > >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8225392 > >>> > >>> Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/ > >>> > >>> /Erik > >>> > >> > > > >
Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file
This is my version of the fix: http://cr.openjdk.java.net/~weijun/8225392/webrev.00/ Now you can still compare cacerts bit by bit. Thanks, Max > On Jun 12, 2019, at 10:50 PM, Weijun Wang wrote: > > Hi Erik, > > Are you going to fix this bug soon? > > I am inspired by Martin's words and would like to update GenerateCacerts.java > so that as long as the certs and their aliases are unchanged, the output > cacerts will always be the same. I can send out a code review today. > > Thanks, > Max > >> On Jun 12, 2019, at 10:59 AM, Weijun Wang wrote: >> >> Good idea about the creation time. >> >> --Max >> >>> On Jun 12, 2019, at 10:53 AM, Martin Buchholz wrote: >>> >>> Google culture really likes build output determinism, and we recently built >>> our own cacerts generator. >>> >>> To get determinism, we are using cert digest as alias (must have a unique >>> alias, but value doesn't seem to matter much), and using cert notBefore >>> instead of current (build) timestamp. >>> >>> On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson >>> wrote: >>> Since JDK-8193255, when we started generating the cacerts file in the >>> build, the build compare baseline builds have started failing. It seems >>> the cacerts binary file has some non determinism built in so it doesn't >>> get generated exactly the same given the same input. This patch adds >>> special handling when comparing that file by comparing the output of >>> "keytool -list" on the files instead. >>> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8225392 >>> >>> Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/ >>> >>> /Erik >>> >> >
Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file
Hi Erik, Are you going to fix this bug soon? I am inspired by Martin's words and would like to update GenerateCacerts.java so that as long as the certs and their aliases are unchanged, the output cacerts will always be the same. I can send out a code review today. Thanks, Max > On Jun 12, 2019, at 10:59 AM, Weijun Wang wrote: > > Good idea about the creation time. > > --Max > >> On Jun 12, 2019, at 10:53 AM, Martin Buchholz wrote: >> >> Google culture really likes build output determinism, and we recently built >> our own cacerts generator. >> >> To get determinism, we are using cert digest as alias (must have a unique >> alias, but value doesn't seem to matter much), and using cert notBefore >> instead of current (build) timestamp. >> >> On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson >> wrote: >> Since JDK-8193255, when we started generating the cacerts file in the >> build, the build compare baseline builds have started failing. It seems >> the cacerts binary file has some non determinism built in so it doesn't >> get generated exactly the same given the same input. This patch adds >> special handling when comparing that file by comparing the output of >> "keytool -list" on the files instead. >> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8225392 >> >> Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/ >> >> /Erik >> >
Re: RFR: 8224851: AArch64: fix warnings and errors with Clang and GCC 8.3
Hi Andrew, Sorry for the delay, I've put an updated webrev here: http://cr.openjdk.java.net/~ngasson/8224851/webrev.1/ Changes since the last patch: * Replaced ~((1<<12) - 1) with -1u<<12 * Use __atomic_add_fetch in Atomic::PlatformAdd #ifdef __clang__ * Use __builtin_frame_address(0) in os::current_frame() * Use placement new / copy assignment in pf() I also replaced the call to _get_previous_fp() in os::current_frame() with *(intptr_t **)__builtin_frame_address(0); As it generates the same code and avoids the `register intptr_t **fp __asm__ (SPELL_REG_FP);' declaration which clang doesn't support. Also the following comment in _get_previous_fp seems to be wrong: // fp is for this frame (_get_previous_fp). We want the fp for the // caller of os::current_frame*(), so go up two frames. However, for // optimized builds, _get_previous_fp() will be inlined, so only go // up 1 frame in that case. #ifdef _NMT_NOINLINE_ return **(intptr_t***)fp; #else return *fp; #endif Even on -O0 gcc won't generate a stack frame for this function so if _NMT_NOINLINE_ is defined you get the caller's caller's FP rather than the caller's FP. I just deleted the function as it's not used anywhere else. BTW we can't use __builtin_frame_address(1) as GCC gives a warning when this is called with a non-zero argument (-Wframe-address). Tested jtreg hotspot_all_no_apps and jdk_core (gcc 7.3). Thanks, Nick On 03/06/2019 19:03, Andrew Haley wrote: Hi, On 6/3/19 11:36 AM, Nick Gasson wrote: We need to know what it's used for to know which solution is right. I'm guessing the primary use case is asynchronous runtime stack probes, used by debugging tools. Yes, we also have os::stack_shadow_pages_available() which uses it to calculate how much space there is between the current SP and the end of the stack. In this case I think it's ok as long as we don't return a value that's *above* the actual stack pointer. And os::current_frame(), which constructs a `frame' object using the FP of the caller, but the SP will point into the frame of os::current_frame(), so it seems it's already inaccurate. Eww. There's also a comment in os_linux.cpp that says "Don't use os::current_stack_pointer(), as its result can be slightly below current stack pointer". So I'm wondering if we can simplify this a lot and use __builtin_frame_address(0) which will give us the FP in os::current_stack_pointer (ought to be the caller's SP - 16). Zero does this currently. And maybe we can replace _get_previous_fp() with __builtin_frame_address(1)? Let's make os::current_stack_pointer() noinline, then make it return __builtin_frame_address(0).
