8332699: ubsan: jfrEventSetting.inline.hpp:31:43: runtime error: index 163 out of bounds for type 'jfrNativeEventSetting [162]'

[MBaesken]

When running hs :tier1 tests or jdk/jfr tests, with ubsan enabled (configure flag --enable-ubsan), in a lot of jfr related tests like
compiler/intrinsics/klass/CastNullCheckDroppingsTest.jtr
serviceability/jvmti/RedefineClasses/RedefineSharedClassJFR.jtr
this oob error can be seen :

/jdk/src/hotspot/share/jfr/recorder/jfrEventSetting.inline.hpp:31:43: runtime error: index 163 out of bounds for type 'jfrNativeEventSetting [162]'
#0 0x7f6b75a8634b in JfrEventSetting::setting(JfrEventId) /jdk/src/hotspot/share/jfr/recorder/jfrEventSetting.inline.hpp:31
#1 0x7f6b75a8634b in JfrEventSetting::set_stacktrace(long, bool) /jdk/src/hotspot/share/jfr/recorder/jfrEventSetting.cpp:47

Looks like the array in generated code is too small.
With
jfrNativeEventSetting bits[NUMBER_OF_EVENTS];
and

static const int NUMBER_OF_EVENTS = 162;
static const int NUMBER_OF_RESERVED_EVENTS = 2;

Access at index 163 cannot work.
But it looks like there is always enough memory after the array so we do not crash and it was not noticed before.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8332699: ubsan: jfrEventSetting.inline.hpp:31:43: runtime error: index 163 out of bounds for type 'jfrNativeEventSetting [162]' (Bug - P4)

Reviewers

• Johannes Bechberger (@parttimenerd - Committer)
• Thomas Stuefe (@tstuefe - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19628/head:pull/19628
$ git checkout pull/19628

Update a local copy of the PR:
$ git checkout pull/19628
$ git pull https://git.openjdk.org/jdk.git pull/19628/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19628

View PR using the GUI difftool:
$ git pr show -t 19628

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19628.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@MBaesken This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8332699: ubsan: jfrEventSetting.inline.hpp:31:43: runtime error: index 163 out of bounds for type 'jfrNativeEventSetting [162]'

Reviewed-by: jbechberger, stuefe

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 29 new commits pushed to the master branch:

• bd046d9: 8222884: ConcurrentClassDescLookup.java times out intermittently
• 1c80ddb: 8333940: Ensure javax/swing/TestUngrab.java run on all platforms
• a7205cc: 8333926: Shenandoah: Lower default immediate garbage threshold
• 56e8e60: 8330534: Update nsk/jdwp tests to use driver instead of othervm
• bbd3b1d: 8334036: Update JCov for class file version 68
• 7ed8a5c: 8333841: Add more logging into setfldw001 tests
• b77bd5f: 8333742: ProcessImpl and ProcessHandleImpl may mishandle processes that exit with code 259
• aaaa86b: 8333360: PrintNullString.java doesn't use float arguments
• ef101f1: 8332920: C2: Partial Peeling is wrongly applied for CmpU with negative limit
• 2843745: 8333972: Parallel: Remove unused methods in PSOldGen
• ... and 19 more: https://git.openjdk.org/jdk/compare/7b43a8cd7c663facbe490f889838d7ead0eba0f9...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@MBaesken The following label will be automatically applied to this pull request:

• build

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[MBaesken]

/label jfr

[mlbridge]

Webrevs

• 00: Full (5cdec3e6)

[openjdk]

@MBaesken
The label jfr is not a valid label.
These labels are valid:

• graal
• serviceability
• hotspot
• hotspot-compiler
• ide-support
• kulla
• i18n
• shenandoah
• jdk
• javadoc
• security
• hotspot-runtime
• jmx
• build
• nio
• client
• core-libs
• compiler
• net
• hotspot-gc
• hotspot-jfr

[parttimenerd]

LGTM

[RealCLanger]

/label hotspot-jfr

[openjdk]

@RealCLanger
The hotspot-jfr label was successfully added.

[tstuefe]

Good catch!

But I am not sure the solution is correct, or not enough.

On my machine (linux x64), I see 169 explicitly named events in JfrNativeSettings::ev. But then, NUMBER_OF_EVENTS = 162; and NUMBER_OF_RESERVED_EVENTS = 2; . => 164. So, the last 5 events are not covered by the bits array even with your change.

So, there is a mismatch somewhere between metadata.getEventsAndStructs() (used to print out individual event data) and metadata.eventCounter.count (used to determine NUMBER_OF_EVENTS).

---

If you have figured this out, and fixed it, could you please let the generator add a

STATIC_ASSERT( sizeof(JfrNativeSettings::ev) == sizeof(JfrNativeSettings::bits) );

to the header?

[MBaesken]

My maximum JfrEventId is 163 , see the generated hotspot/variant-server/gensrc/jfrfiles/jfrEventIds.hpp

enum JfrEventId {
  JfrMetadataEvent = 0,
  JfrCheckpointEvent = 1,
  JfrDurationEvent = 2,
  JfrInstantEvent = 3,
  JfrValueEvent = 4,
  JfrTextEvent = 5,
  JfrZThreadDebugEvent = 6,
   .....
  JfrJavaAgentEvent = 161,
  JfrNativeAgentEvent = 162,
  JfrDeprecatedInvocationEvent = 163,
};

so NUMBER_OF_EVENTS + NUMBER_OF_RESERVED_EVENTS looks fine to me.
163 is the highest I could see while testing.

[tstuefe]

My maximum JfrEventId is 163 , see the generated hotspot/variant-server/gensrc/jfrfiles/jfrEventIds.hpp

enum JfrEventId {
  JfrMetadataEvent = 0,
  JfrCheckpointEvent = 1,
  JfrDurationEvent = 2,
  JfrInstantEvent = 3,
  JfrValueEvent = 4,
  JfrTextEvent = 5,
  JfrZThreadDebugEvent = 6,
   .....
  JfrJavaAgentEvent = 161,
  JfrNativeAgentEvent = 162,
  JfrDeprecatedInvocationEvent = 163,
};

so NUMBER_OF_EVENTS + NUMBER_OF_RESERVED_EVENTS looks fine to me. 163 is the highest I could see while testing.

No, I am quite sure the ev substructure is wrong. The structure contains members that are not events, that's why its larger.

But it's an issue orthogonal to the one you are fixing. I opened https://bugs.openjdk.org/browse/JDK-8334031 to track it.

[tstuefe]

About your fix, do you know why MetadataEvent and CheckPointEvent would not count toward the number of events? In other words, why NUMBER_OF_EVENTS is 162 if we have 164 events? Maybe the number of events is wrong?

@egahlin ?

[egahlin]

About your fix, do you know why MetadataEvent and CheckPointEvent would not count toward the number of events? In other words, why NUMBER_OF_EVENTS is 162 if we have 164 events? Maybe the number of events is wrong?

@egahlin ?

The metadata and check point events are built-in and hold labels/descriptions and constants, so they can't be enabled/disabled and thus don't need event settings.

[tstuefe]

@egahlin thank you. Patch is okay then.

[MBaesken]

Hi Thomas and Johannes, thanks for the reviews !

/integrate

[openjdk]

Going to push as commit abbf45b.
Since your change was applied there have been 29 commits pushed to the master branch:

• bd046d9: 8222884: ConcurrentClassDescLookup.java times out intermittently
• 1c80ddb: 8333940: Ensure javax/swing/TestUngrab.java run on all platforms
• a7205cc: 8333926: Shenandoah: Lower default immediate garbage threshold
• 56e8e60: 8330534: Update nsk/jdwp tests to use driver instead of othervm
• bbd3b1d: 8334036: Update JCov for class file version 68
• 7ed8a5c: 8333841: Add more logging into setfldw001 tests
• b77bd5f: 8333742: ProcessImpl and ProcessHandleImpl may mishandle processes that exit with code 259
• aaaa86b: 8333360: PrintNullString.java doesn't use float arguments
• ef101f1: 8332920: C2: Partial Peeling is wrongly applied for CmpU with negative limit
• 2843745: 8333972: Parallel: Remove unused methods in PSOldGen
• ... and 19 more: https://git.openjdk.org/jdk/compare/7b43a8cd7c663facbe490f889838d7ead0eba0f9...master

Your commit was automatically rebased without conflicts.

[openjdk]

@MBaesken Pushed as commit abbf45b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.