Re: RFR: 8279918: Fix various doc typos [v2]
On Thu, 13 Jan 2022 14:01:04 GMT, Pavel Rappo wrote: >> - Most of the typos are of a trivial kind: missing whitespace. >> - If any of the typos should be fixed in the upstream projects instead, >> please say so; I will drop those typos from the patch. >> - As I understand it, ` ` in ImageInputStream and DataInput is an irrelevant >> formatting artefact and thus can be removed. >> - `` is an apostrophe, which does not require to be encoded. > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Additional typos Marked as reviewed by egahlin (Reviewer). The JFR related changes looks OK. - PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8266936: Add a finalization JFR event [v2]
On Tue, 18 May 2021 22:41:06 GMT, Brent Christian wrote: >> Please review this enhancement to add a new JFR event, generated whenever a >> finalizer is run. >> (The makeup is similar to the Deserialization event, >> [JDK-8261160](https://bugs.openjdk.java.net/browse/JDK-8261160).) >> >> The event's only datum (beyond those common to all jfr events) is the class >> of the object that was finalized. >> >> The Category for the event: >> `"Java Virtual Machine" / "GC" / "Finalization"` >> is what made sense to me, even though the event is generated from library >> code. >> >> Along with the new regtest, I added a run mode to the basic finalizer test >> to enable jfr. >> Automated testing looks good so far. >> >> Thanks, >> -Brent > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > Test flag should be volatile I wonder if there needs to be one event per finalized object? Perhaps a counter per class would be as useful, i.e. jdk.FinalizationStatistics, and if it could be implemented in the VM, without other implications, that would be great. Such an event could be enabled by default and provide much greater value than an event that users would need to know about and configure themselves, which 99,99% of all user will not do. - PR: https://git.openjdk.java.net/jdk/pull/4101
Re: RFR: 8266936: Add a finalization JFR event [v2]
On Tue, 18 May 2021 22:41:06 GMT, Brent Christian wrote: >> Please review this enhancement to add a new JFR event, generated whenever a >> finalizer is run. >> (The makeup is similar to the Deserialization event, >> [JDK-8261160](https://bugs.openjdk.java.net/browse/JDK-8261160).) >> >> The event's only datum (beyond those common to all jfr events) is the class >> of the object that was finalized. >> >> The Category for the event: >> `"Java Virtual Machine" / "GC" / "Finalization"` >> is what made sense to me, even though the event is generated from library >> code. >> >> Along with the new regtest, I added a run mode to the basic finalizer test >> to enable jfr. >> Automated testing looks good so far. >> >> Thanks, >> -Brent > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > Test flag should be volatile This looks useful, but I am worried about the performance impact: - The added allocation for every object that is finalized. - The event being enabled in the default configuration. The default configuration must be safe to use even in pathological cases, i.e. an application with lots of objects being finalized. It's probably too much overhead (or number of events) for the profile configuration as well. There is also the added startup cost of JFR. One event may not matter that much, but they all add up. We need to put in place a mechanism that doesn't rely on bytecode instrumentation at runtime. There is an enhancement for that, but it requires some engineering first. - PR: https://git.openjdk.java.net/jdk/pull/4101
Re: RFR: 8261160: Add a deserialization JFR event [v5]
On Fri, 12 Feb 2021 16:26:09 GMT, Chris Hegarty wrote: >> This issue adds a new event to improve diagnostic information of Java >> deserialization. The event captures the details of deserialization activity >> from ObjectInputStream. The event details are similar to that of the serial >> filter, but is agnostic of whether a filter is installed or not. The event >> also captures the filter status, if there is one. > > Chris Hegarty has updated the pull request incrementally with one additional > commit since the last revision: > > Split exception into type and message Marked as reviewed by egahlin (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event
On Tue, 9 Feb 2021 12:35:27 GMT, Chris Hegarty wrote:
> This issue adds a new event to improve diagnostic information of Java
> deserialization. The event captures the details of deserialization activity
> from ObjectInputStream. The event details are similar to that of the serial
> filter, but is agnostic of whether a filter is installed or not. The event
> also captures the filter status, if there is one.
src/java.base/share/classes/java/io/ObjectInputStream.java line 1366:
> 1364: DeserializationEvent event = new DeserializationEvent();
> 1365: if (event.shouldCommit()) {
> 1366: event.filterStatus = status == null ? "n/a" : status.name();
We use null for missing value, so no need to have "n/a"
src/java.base/share/classes/java/io/ObjectInputStream.java line 1372:
> 1370: event.depth = depth;
> 1371: event.bytesRead = bytesRead;
> 1372: event.exception = Objects.toString(ex, "n/a");
You may want to change the name of the field to "exceptionMessage" to make it
clear it's the message, not the class.
src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 45:
> 43:
> 44: @Label ("Class")
> 45: public String clazz;
We typically use "type" when referring to a class.
src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 45:
> 43:
> 44: @Label ("Class")
> 45: public String clazz;
Is it possible to make the field of type Class?
src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 51:
> 49:
> 50: @Label ("Reference count")
> 51: public long totalObjectRefs;
"Reference count" sounds a bit like resource counter? Is that the case? If not,
perhaps "Object References" is better. We tried to avoid abbreviations. How
about naming the field "totalObjectReferences" or just "objectReferences"?
-
PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8138732: Rename @HotSpotIntrinsicCandidate to @IntrinsicCandidate and move it to the jdk.internal.vm.annotation package [v5]
On Wed, 23 Sep 2020 18:41:06 GMT, Philippe Marschall wrote: >> Hello, newbie here >> >> I picked JDK-8138732 to work on because it has a "starter" label and I >> believe I understand what to do. >> >> - I tried to update the copyright year to 2020 in every file. >> - I decided to change `@since` from 9 to 16 since it is a new annotation >> name in a new package. >> - I tried to keep code changes to a minimum, eg not change to imports if >> fully qualified class names are used instead of >> imports. In some cases I did minor reordering of imports to keep them >> sorted alphabetically. >> - All tier1 tests pass. >> - One jpackage/jlink tier2 test fails but I don't believe it is related. >> - Some tier3 Swing tests fail but I don't think they are related. > > Philippe Marschall has updated the pull request with a new target base due to > a merge or a rebase. The pull request now > contains one commit: > 8138732: Rename HotSpotIntrinsicCandidate to IntrinsicCandidate Marked as reviewed by egahlin (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/45
Re: RFR: 8138732: Rename @HotSpotIntrinsicCandidate to @IntrinsicCandidate and move it to the jdk.internal.vm.annotation package [v2]
On Sat, 12 Sep 2020 00:19:00 GMT, Vladimir Kozlov wrote: >> Philippe Marschall has refreshed the contents of this pull request, and >> previous commits have been removed. The >> incremental views will show differences compared to the previous content of >> the PR. The pull request contains one new >> commit since the last revision: >> 8138732: Rename HotSpotIntrinsicCandidate to IntrinsicCandidate > > Marked as reviewed by kvn (Reviewer). Have you run the JFR tests in test/jdk/jdk/jfr? - PR: https://git.openjdk.java.net/jdk/pull/45
Re: 8215534: [testbug] some jfr test don't check @requires vm.hasJFR
Looks good. Erik Hi, These tests lack @requires vm.hasJFR, thus they are failing on AIX. http://cr.openjdk.java.net/~goetz/wr18/8215334-JFR_requires/01/ Please review. I will push this to jdk12 as it is a testbug if I miss the RDP deadline. Best regards, Goetz.
Re: RFR: 8148188: Enhance the security libraries to record events of interest
Hi Sean, I think we could still call the event "jdk.SecurityPropertyModification", but add a @Description that explains that events are only emitted for the JDK due to security concerns. If we at a later stage want to include user events, we could add those and remove the @Description, possibly with a setting where you can specify scope, or perhaps a new event all together. Perhaps we could find a better name for the field validationEventNumber? It sounds like we have an event number, which is not really the case. We have a counter for the validation id. I noticed that you use hashCode as an id. I assume this is to simplify the implementation? That is probably OK, the risk of a collision is perhaps low, but could you make the field a long, so we in the future could add something more unique (Flight Recorder uses compressed integers, so a long uses the same amount of disk space as an int). Could you also rename the field and the annotation, perhaps certificationId could work, so we don't leak how the id was implemented. I could not find the file: test/lib/jdk/test/lib/security/TestJdkSecurityPropertyModification.java Thanks Erik With JDK-8203629 now pushed, I've re-based my patch on latest jdk/jdk code. Some modifications also made based on off-thread suggestions : src/java.base/share/classes/java/security/Security.java * Only record JDK security properties for now (i.e. those in java.security conf file) - we can consider a new event to record non-JDK security events if demand comes about - new event renamed to *Jdk*SecurityPropertyModificationEvent src/java.base/share/classes/sun/security/x509/X509CertImpl.java * Use hashCode() equality test for storing certs in List. Tests updated to capture these changes src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java * Verify that self signed certs exercise the modified code paths I've added new test functionality to test use of self signed cert. webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v7/webrev/ Regards, Sean. On 17/10/18 11:25, Seán Coffey wrote: I'd like to re-boot this review. I've been working with Erik off thread who's been helping with code and test design. Some of the main changes worthy of highlighting : * Separate out the tests for JFR and Logger events * Rename some events * Normalize the data view for X509ValidationEvent (old event name : CertChainEvent) * Introduce CertificateSerialNumber type to make use of the @Relational JFR annotation. * Keep calls for JFR event operations local to call site to optimize jvm compilation webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v6/webrev/ This code is dependent on the JDK-8203629 enhancement being integrated. Regards, Sean. On 10/07/18 13:50, Seán Coffey wrote: Erik, After some trial edits, I'm not so sure if moving the event & logger commit code into the class where it's used works too well after all. In the code you suggested, there's an assumption that calls such as EventHelper.certificateChain(..) are low cost. While that might be the case here, it's something we can't always assume. It's called once for the JFR operation and once for the Logger operation. That pattern multiplies itself further in other Events. i.e. set up and assign the variables for a JFR event without knowing whether they'll be needed again for the Logger call. We could work around it by setting up some local variables and re-using them in the Logger code but then, we're back to where we were. The current EventHelper design eliminates this problem and also helps to abstract the recording/logging functionality away from the functionality of the class itself. It also becomes a problem where we record events in multiple different classes (e.g. SecurityPropertyEvent). While we could leave the code in EventHelper for this case, we then have a mixed design pattern. Are you ok with leaving as is for now? A future wish might be one where JFR would handle Logger via its own framework/API in a future JDK release. I've removed the variable names using underscore. Also optimized some variable assignments in X509Impl.commitEvent(..) http://cr.openjdk.java.net/~coffeys/webrev.8148188.v5/webrev/ regards, Sean. On 09/07/2018 18:01, Seán Coffey wrote: Erik, Thanks for reviewing. Comments inline.. On 09/07/18 17:21, Erik Gahlin wrote: Thanks Sean. Some feedback on the code in the security libraries. - We should use camel case naming convention for variables (not underscore). Sure. I see two offending variable names which I'll fix up. - Looking at sun/security/ssl/Finished.java, I wonder if it wouldn't be less code and more easy to read, if we would commit the event in a local private function and then use the EventHelper class for common functionality. I'm fine with your suggested approach. I figured that tucking the recording/logging logic away in a helper class als
Re: RFR: 8148188: Enhance the security libraries to record events of interest
Thanks Sean.
Some feedback on the code in the security libraries.
- We should use camel case naming convention for variables (not underscore).
- Looking at sun/security/ssl/Finished.java,
I wonder if it wouldn't be less code and more easy to read, if we would
commit the event in a local private function and then use the
EventHelper class for common functionality. Instead of:
private static void recordEvent(SSLSessionImpl session) {
TLSHandshakeEvent hs_event = new TLSHandshakeEvent();
if (hs_event.isEnabled() || EventHelper.loggingSecurity()) {
String certIDs = "";
try {
certIDs = Stream.of(session.getPeerCertificates())
.filter(c -> c instanceof X509Certificate)
.map(c -> (X509Certificate) c)
.map(c -> c.getSerialNumber().toString(16))
.collect(Collectors.joining(", "));
} catch (SSLPeerUnverifiedException e) {
certIDs = e.getMessage(); // not verified msg
}
EventHelper.commitTLSHandshakeEvent(hs_event, null,
session.getPeerHost(), session.getPeerPort(),
session.getCipherSuite(), session.getProtocol(),
certIDs);
}
}
...
public static void commitTLSHandshakeEvent(TLSHandshakeEvent event,
Instant start,
String remoteHost,
int remotePort,
String cipherSuite,
String protocolVersion,
String certChain) {
if (event != null && event.shouldCommit()) {
event.remoteHost = remoteHost;
event.remotePort = remotePort;
event.cipherSuite = cipherSuite;
event.protocolVersion = protocolVersion;
event.certificateChain = certChain;
event.commit();
}
if (loggingSecurity()) {
String prepend = getDurationString(start);
SECURITY_LOGGER.log(LOG_LEVEL, prepend +
" TLSHandshake: {0}:{1,number,#}, {2}, {3}, {4}",
remoteHost, remotePort, protocolVersion, cipherSuite,
certChain);
}
}
We would do:
private static void logHandshake(SSLSessionImpl s) {
TLSHandshakeEvent event = new TLSHandshakeEvent();
if (event.shouldCommit()) {
event.remoteHost = s.getPeerHost();
event.remotePort = s.getPeerPort();
event.cipherSuite = s.getCipherSuite();
event.protocolVersion = s.getProtocol();
event.certificateChain =
EventHelper.certificateChain(s.getPeerCerticates());
event.commit();
}
if (EventHelper.isLoggingSecurity()) {
EventHelper.SECURITY_LOGGER.log(LOG_LEVEL, " TLSHandshake:
{0}:{1,number,#}, {2}, {3}, {4}",
s.getPeerHost(), s.getPeerPort(), s.getProtocol(),
s.getCipherSuite(),
EventHelper.certificateChain(s.getPeerCerticates()));
}
}
...
public static String certificateChain(Certificate[] certificates) {
try {
return Stream.of(certificates)
.filter(c -> c instanceof X509Certificate)
.map(c -> (X509Certificate) c)
.map(c -> c.getSerialNumber().toString(16))
.collect(Collectors.joining(", "));
} catch (SSLPeerUnverifiedException e) {
return e.getMessage(); // not verified msg
}
}
It will also meanless overhead, since only one check is needed for the
log and the JIT will be able to remove the allocation.
Maybe a similar pattern could be used for the other events as well?
Thanks
Erik
As per request from Erik, I separated the tests out into individual
ones to test the JFR and Logger functionality. I introduced a new
separate test for the CertificateChainEvent event also. Originally
this was wrapped into the TLSHandshakeEvent test.
Thanks to Erik for extra refactoring and modifications carried out to
clean up the code up further.
http://cr.openjdk.java.net/~coffeys/webrev.8148188.v5/webrev/
This enhancement has a dependency on JDK-8203629
Regards,
Sean.
On 02/07/18 09:49, Erik Gahlin wrote:
On 29 Jun 2018, at 17:34, Seán Coffey wrote:
I've introduced a new test helper class in the jdk/test/lib/jfr
directory to help with the dual test nature of the new tests. It's
helped alot with test code duplication.
My thinking was to put things like the certificates in a separate
file, i.e TestCertificates, and then have a logging test and a JFR
test reuse it.
One rationale for adding logging was to use it if JFR is not present.
By putting the tests together, it becomes impossible to compile and
test logging without having JFR.
Looked at use of @DataAmount(DataAmou
Re: RFR: 8148188: Enhance the security libraries to record events of interest
> On 29 Jun 2018, at 17:34, Seán Coffey wrote:
>
> I've introduced a new test helper class in the jdk/test/lib/jfr directory to
> help with the dual test nature of the new tests. It's helped alot with test
> code duplication.
>
My thinking was to put things like the certificates in a separate file, i.e
TestCertificates, and then have a logging test and a JFR test reuse it.
One rationale for adding logging was to use it if JFR is not present. By
putting the tests together, it becomes impossible to compile and test logging
without having JFR.
> Looked at use of @DataAmount(DataAmount.BITS) also. Not sure if it's fits.
> The output is displayed in units like "KiB" - not the normal when examining
> key lengths used in X509Certificates. i.e a 2048 bit key gets displayed as "2
> KiB" - I'd prefer to keep the 2048 display version.
We should not let the JMC GUI decide how units are specified. There will be
other GUIs and this is the first event that uses bits, so I don’t think it is
formatted that way because it was considered superior.
Erik
>
> new webrev at: http://cr.openjdk.java.net/~coffeys/webrev.8148188.v4/webrev/
>
> Regards,
> Sean.
>
> On 28/06/18 17:59, Seán Coffey wrote:
>> Comments inline.
>>
>>
>> On 28/06/2018 17:20, Erik Gahlin wrote:
>>> It's sufficient if an event object escapes to another method (regardless if
>>> JFR is enabled or not).
>>>
>>> Some more feedback:
>>>
>>> Rename event jdk.CertChain to jdk.CertificateChain
>>> Rename event jdk.X509Cert to jdk.X509Certificate
>>> Rename field certChain to certificateChain.
>>> Rename field serialNum to serialNumber
>> all above done.
>>> Rename field algId to AlgorithmicId or AlgorithmicID
>> maybe "algorithm" works here also ?
>>> Rename @Label("Ciphersuite") to @Label("Cipher Suite")
>>> Rename @Label("Cert Chain") to @Label("Certificate Chain")
>>> Rename @Label("Property Name") to "Name" or "Key" if that makes sense in
>>> the context?
>>> Rename @Label("Property Value") to "Value".
>>> Put events in a subcategory, i.e @Category({"Java Development Kit",
>>> "Security"})
>> done.
>>> Make classes final.
>> done. I had thought that the JFR mechanism required non-final classes.
>>> What is the unit of the key in X509Certificate event? Bits? If that is the
>>> case, use @DataAmount(DataAmount.BITS)
>> Yes - it's essentially the bit length of the keys used. Let me look into
>> that annotation usage.
>>> @Label("Serial numbers forming chain of trust") should not be a sentence.
>>> How about "Serial Numbers"?
>>>
>>> I think the tests are hard to read when two things are tested at the same
>>> time. It is also likely that if a test fail due to logging issues, it will
>>> be assigned to JFR because of the test name, even thought the issues is not
>>> JFR related.
>> I think we're always going to have some ownership issues when tests serve a
>> dual purpose. I still think it makes sense to keep the test logic in one
>> place. Any failures in these tests will most likely be owned by security
>> team. (moving the tests to security directory is also an option)
>>>
>>> If you want to reuse code between tests, I would put it in testlibrary.
>> Let me check if there's any common patterns that could be added to the
>> testlibary.
>>
>> Thanks,
>> Sean.
>>>
>>> Erik
>>>
>>>> Thanks for the update Erik. By default I'm proposing that the new JFR
>>>> Events and Logger be disabled. As a result the event class shouldn't
>>>> escape. If performance metrics highlight an issue, we should revisit.
>>>>
>>>> regards,
>>>> Sean.
>>>>
>>>>
>>>> On 27/06/2018 20:57, Erik Gahlin wrote:
>>>>> On 2018-06-27 21:14, Seán Coffey wrote:
>>>>>>
>>>>>>
>>>>>> On 27/06/2018 19:57, Xuelei Fan wrote:
>>>>>>> Hi Sean,
>>>>>>>
>>>>>>> I may reply in several replies.
>>>>>>>
>>>>>>> PKIXMasterCertPathValidator.java
>>>>>>>
>>>>>>> + CertChainEvent cce = new CertChainEvent();
>>>>>>> + if(cce.isEnabled() || EventHelper.loggingSecurity(
Re: RFR: 8148188: Enhance the security libraries to record events of interest
It's sufficient if an event object escapes to another method (regardless
if JFR is enabled or not).
Some more feedback:
Rename event jdk.CertChain to jdk.CertificateChain
Rename event jdk.X509Cert to jdk.X509Certificate
Rename field certChain to certificateChain.
Rename field serialNum to serialNumber
Rename field algId to AlgorithmicId or AlgorithmicID
Rename @Label("Ciphersuite") to @Label("Cipher Suite")
Rename @Label("Cert Chain") to @Label("Certificate Chain")
Rename @Label("Property Name") to "Name" or "Key" if that makes sense in
the context?
Rename @Label("Property Value") to "Value".
Put events in a subcategory, i.e @Category({"Java Development Kit",
"Security"})
Make classes final.
What is the unit of the key in X509Certificate event? Bits? If that is
the case, use @DataAmount(DataAmount.BITS)
@Label("Serial numbers forming chain of trust") should not be a
sentence. How about "Serial Numbers"?
I think the tests are hard to read when two things are tested at the
same time. It is also likely that if a test fail due to logging issues,
it will be assigned to JFR because of the test name, even thought the
issues is not JFR related.
If you want to reuse code between tests, I would put it in testlibrary.
Erik
Thanks for the update Erik. By default I'm proposing that the new JFR
Events and Logger be disabled. As a result the event class shouldn't
escape. If performance metrics highlight an issue, we should revisit.
regards,
Sean.
On 27/06/2018 20:57, Erik Gahlin wrote:
On 2018-06-27 21:14, Seán Coffey wrote:
On 27/06/2018 19:57, Xuelei Fan wrote:
Hi Sean,
I may reply in several replies.
PKIXMasterCertPathValidator.java
+ CertChainEvent cce = new CertChainEvent();
+ if(cce.isEnabled() || EventHelper.loggingSecurity()) {
+ String c = reversedCertList.stream()
+ .map(x -> x.getSerialNumber().toString(16))
+.collect(Collectors.joining(", "));
+ EventHelper.commitCertChainEvent(cce, c);
+ }
No matter the event or the JFR mechanism is enabled or not, the
above code will create a new instance. Is the return value of
cce.isEnabled() dynamically changed or static?
This is a requirement from the JFR framework. All their
event.isEnabled calls are instance methods and follow a similar
pattern. The JFR team assure me that the JIT can optimize away such
calls.
The JIT will most likely not be able to optimize if the event class
escapes.
From a JFR perspective, this would be the preferred layout:
EventA eventA= new EventA();
eventA.value = this.value;
eventA.commit();
and then do logging separately:
System.Logger.log(DEBUG, this.value)
With this layout, the JIT will remove the allocation and dead store.
If it is expensive to gather the data for the event, like the
CertChainEvent above, the following pattern should be used.
EventB eventB= new EventB ();
if (eventB.shouldCommit()) {
eventB.value = calculateValue();
eventB .commit();
}
If JFR is not enabled, shouldCommit returns false and the JIT should
be able to remove the allocation. This will be best from a
performance point of view, and also in my opinion from a maintenance
and readability perspective. Others may disagree.
Erik
Is there a need to support both logging and JFR? I'm new to record
events. I did not get the point to use both.
I was initially hoping to concentrate on just JFR events but I got
strong feedback to also consider use of Logger (esp. in cases where
the jdk.jfr module is not available)
regards,
Sean.
Thanks,
Xuelei
On 6/26/2018 3:18 PM, Seán Coffey wrote:
Erik,
I rebased the patch with TLS v1.3 integration today. I hadn't
realized how much the handshaker code had changed. Hopefully, I'll
get a review from security dev team on that front.
Regards the JFR semantics, I believe the edits should match
majority of requests . I still have a preference for the test
infra design for these new logger/JFR tests used in version 1 of
webrev. I think it makes sense to keep the test functionality
together - no sense in separating them out into different
components IMO. Also, some of the edits to the JFR testing were
made to test for the new dual log/record functionality. I might
catch up with you tomorrow to see what the best arrangement would be.
http://cr.openjdk.java.net/~coffeys/webrev.8148188.v2/webrev/
regards,
Sean.
On 25/06/2018 21:22, Seán Coffey wrote:
Many thanks for the review comments Erik. Replies inline.
On 24/06/2018 14:21, Erik Gahlin wrote:
Hi Sean,
Some of the changes in the webrev belongs to JDK-8203629 and
should be removed for clarity.
Some initial comments:
default.jfc, profile.jfr:
The events should not have control="enable-exceptions". The
purpose of the control attribute is so to provide pa
Re: RFR: 8148188: Enhance the security libraries to record events of interest
Hi Sean,
Some of the changes in the webrev belongs to JDK-8203629 and should be
removed for clarity.
Some initial comments:
default.jfc, profile.jfr:
The events should not have control="enable-exceptions". The purpose of
the control attribute is so to provide parameterized configuration of
events for JMC. As it is now, the security events will be enabled when
a user turns on the exception events.
X509CertEvent:
You should use milliseconds since epoch to represent a date instead of a
string value, i.e.
@Label("Valid From")
@Timestamp(Timestamp.MILLISECONDS_SINCE_EPOCH)
public long validFrom;
@Label("Valid Until")
@Timestamp(Timestamp.MILLISECONDS_SINCE_EPOCH)
public long validUntil;
This following annotation adds little value
@Description("Details of Security Property")
I would either remove the annotation, or provide information that helps
a user understand the event. For instance, what is X509, and what kind
of certificates are we talking about?
@Category("Java Application")
I'm a bit worried that we will pollute the "Java Application" namespace
if we add lots of JDK events in that category. We may want to add a new
top level category "Java Development Kit", analogous to the "Java
Virtual Machine" category, and put all security related events in
subcategory, perhaps called "Security".
@Label("X509Cert")
The label should be human readable name, with spaces, title cased etc. I
would recommend "X.509 Certificate". In general, avoid abbreviations
like "certs" and instead use labels such as "Certificate Chain". The
label should be short and not a full sentence.
For details see,
https://docs.oracle.com/javase/10/docs/api/jdk/jfr/Label.html
I think it would be good to separate testing of JFR and logging into
different files / tests. I would prefer that the test name is the same
as the event prefixed with "Test", i.e TestX509CertificateEvent, as this
is the pattern used by other JFR tests.
I reworked one of the tests to how I like to see it:
/*
* @test
* @key jfr
* @library /test/lib
* @run main/othervm jdk.jfr.event.security.TestX509CertificateEvent
*/
public class TestX509CertificateEvent {
private static final String CERTIFICATE_1 = "...";
private static final String CERTIFICATE_2 = "...";
public static void main(String... args) throws CertificateException {
Recording r = new Recording();
r.enable(EventNames.X509Certificate).withoutStackTrace();
r.start();
CertificateFactory cf = CertificateFactory.getInstance("X.509");
cf.generateCertificate(new
ByteArrayInputStream(CERTIFICATE_1.getBytes()));
cf.generateCertificate(new
ByteArrayInputStream(CERTIFICATE_2.getBytes()));
// Make sure only one event per certificate
cf.generateCertificate(new
ByteArrayInputStream(CERTIFICATE_1.getBytes()));
cf.generateCertificate(new
ByteArrayInputStream(CERTIFICATE_2.getBytes()));
r.stop();
List events = Events.fromRecording(r);
Asserts.assertEquals(events.size(), 2, "Expected two X.509
Certificate events");
assertEvent(events, "1000", "SHA256withRSA",
"CN=SSLCertificate, O=SomeCompany",
"CN=Intermediate CA Cert, O=SomeCompany",
"RSA", 2048);
assertEvent(events, "1000", "SHA256withRSA",
"CN=SSLCertificate, O=SomeCompany",
"CN=Intermediate CA Cert, O=SomeCompany",
"RSA", 2048);
}
private static void assertEvent(List events, String
certID, String algId, String subject,
String issuer, String keyType, int length) throws Exception {
for (RecordedEvent e : events) {
if (e.getString("serialNumber").equals(certID)) {
Events.assertField(e, "algId").equal(algId);
...
return;
}
}
System.out.println(events);
throw new Exception("Could not find event with Cert ID: " +
certID);
}
}
The reworked example uses the Events.assertField method, which will give
context if the assertion fails. Keeping the test simple, means it can be
analyzed quickly if it fails in testing. There is no new test framework
to learn, or methods to search for, and it is usually not hard to
determine if the failure is product, test or infrastructure related, and
what component (team) should be assigned the issue. The use of
EventNames.X509Certificate means all occurrences of the event can be
tracked done in an IDE using find by reference.
Thanks
Erik
Following on from the recent JDK-8203629 code review, I'd like to
propose enhancements on how we can record events in security libs. The
introduction of the JFR libraries can give us much better ways of
examining JDK actions. For the initial phase, I'm looking to enhance
some key security library events in JDK 11 so that they can be either
RFR(S): 8047368: Remove oracle.jrockit.jfr from open package.access list
Hi, Could I have a review of a small fix that removes references to jfr from the package.access list. Bug: https://bugs.openjdk.java.net/browse/JDK-8047368 Webrev: http://cr.openjdk.java.net/~egahlin/8047368/ Thanks Erik
hg: jdk8/tl/jdk: 7141544: TEST_BUG: com/sun/jdi/BreakpointWithFullGC.sh fails
Changeset: 894a4bae9e33 Author:egahlin Date: 2013-11-20 12:32 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/894a4bae9e33 7141544: TEST_BUG: com/sun/jdi/BreakpointWithFullGC.sh fails Reviewed-by: sla ! test/com/sun/jdi/BreakpointWithFullGC.sh
hg: jdk8/tl/jdk: 8028505: Put sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.sh on ProblemList.txt
Changeset: d6195774dd1f Author:egahlin Date: 2013-11-19 11:47 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d6195774dd1f 8028505: Put sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.sh on ProblemList.txt Reviewed-by: alanb ! test/ProblemList.txt
hg: jdk8/tl/jdk: 6959636: testcase failing on windows javax/management/loading/LibraryLoader/LibraryLoaderTest.java
Changeset: ddaa9a8acaed Author:egahlin Date: 2013-11-13 15:21 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ddaa9a8acaed 6959636: testcase failing on windows javax/management/loading/LibraryLoader/LibraryLoaderTest.java Reviewed-by: sla, jbachorik ! test/ProblemList.txt ! test/javax/management/loading/LibraryLoader/LibraryLoaderTest.java
hg: jdk8/tl/jdk: 6954510: TEST_BUG: Testcase failure com/sun/jdi/BreakpointWithFullGC.sh
Changeset: 256b3395346b Author:egahlin Date: 2013-11-13 18:41 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/256b3395346b 6954510: TEST_BUG: Testcase failure com/sun/jdi/BreakpointWithFullGC.sh Reviewed-by: sla, sspitsyn ! test/com/sun/jdi/BreakpointWithFullGC.sh
hg: jdk8/tl/jdk: 8027209: javax/management/monitor/ThreadPoolAccTest.java fails intermittently
Changeset: 41dcb0c2e194 Author:egahlin Date: 2013-11-12 14:52 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/41dcb0c2e194 8027209: javax/management/monitor/ThreadPoolAccTest.java fails intermittently Reviewed-by: sla, jbachorik ! test/javax/management/monitor/ThreadPoolAccTest.java
hg: jdk8/tl/jdk: 6543856: MonitorVmStartTerminate.sh fails intermittently
Changeset: 4cff9f59644f Author:egahlin Date: 2013-11-12 17:40 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4cff9f59644f 6543856: MonitorVmStartTerminate.sh fails intermittently Reviewed-by: sla, dholmes ! test/sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.java ! test/sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.sh ! test/sun/jvmstat/testlibrary/JavaProcess.java
hg: jdk8/tl/jdk: 6849945: VM Periodic Task Thread CPU time = -1ns in HotspotThreadMBean.getInternalThreadCpuTimes()
Changeset: d9f827e4d20c Author:egahlin Date: 2013-11-12 18:12 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d9f827e4d20c 6849945: VM Periodic Task Thread CPU time = -1ns in HotspotThreadMBean.getInternalThreadCpuTimes() Reviewed-by: sla ! test/sun/management/HotspotThreadMBean/GetInternalThreads.java
hg: jdk8/tl/jdk: 7105883: JDWP: agent crash if there exists a ThreadGroup with null name
Changeset: c077a2810782 Author:egahlin Date: 2013-10-23 10:50 +0200 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c077a2810782 7105883: JDWP: agent crash if there exists a ThreadGroup with null name Reviewed-by: sla, jbachorik ! src/share/back/ThreadGroupReferenceImpl.c + test/com/sun/jdi/NullThreadGroupNameTest.java
