Skip to content

Conversation

@mrserb
Copy link
Member

@mrserb mrserb commented Nov 9, 2020

The JavaSound supports the special system exclusive message(SysexMessage).

An important part of the spec:

Data of a system exclusive message should be stored in the data array of a {@code SysexMessage} as follows: the system exclusive message status byte (0xF0 or 0xF7), all message data bytes, and finally the end-of-exclusive flag (0xF7):

The first {@code SysexMessage} object containing data for a particular system
exclusive message should have the status value 0xF0. If this message contains
all the system exclusive data for the message, it should end with the status
byte 0xF7 (EOX). Otherwise, additional system exclusive data should be sent
in one or more {@code SysexMessages} with a status value of 0xF7. The
{@code SysexMessage} containing the last of the data for the system exclusive
message should end with the value 0xF7 (EOX) to mark the end of the system
exclusive message.

In short, the text above can be represented by these examples:

  1. SImple case: SysexMessage{0xF0, some_data, 0xF7}
  2. "Continuation" sysex messages: SysexMessage{0xF0,some_data}, SysexMessage{0xF7,some_data}, SysexMessage{0xF7,some_data}, SysexMessage{0xF7,some_data, 0xF7}.

Note that the second case above the "SysexMessage{0xF7,some_data}" is named as a "continuation" sysex messages.
Usually, when a create a sysex message we carefully calculate the size of the message before sending it to the native code, but the "continuation" sysex messages were implemented in 2003 directly in native after all checks are done, and it just skips the status byte and tries to push nonexistent data to the native device.

So the culprit is in the message like this:
SysexMessage{0xF0,some_data}, SysexMessage{0xF7}.

The code assumes that the second message is "continuation", but it does not, it just ends the previous message.

After the fix, we will not consider the 0xF7 as a continuation if there are no data after.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ❌ (1/9 failed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Failed test task

Issue

  • JDK-8237495: Java MIDI fails with a dereferenced memory error when asked to send a raw 0xF7

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1135/head:pull/1135
$ git checkout pull/1135

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 9, 2020

👋 Welcome back serb! 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
Copy link

openjdk bot commented Nov 9, 2020

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

  • sound

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.

@openjdk openjdk bot added the sound client-libs-dev@openjdk.org label Nov 9, 2020
@mrserb mrserb marked this pull request as ready for review November 9, 2020 20:23
@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 9, 2020
@mlbridge
Copy link

mlbridge bot commented Nov 9, 2020

Webrevs

@openjdk
Copy link

openjdk bot commented Nov 9, 2020

@mrserb 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:

8237495: Java MIDI fails with a dereferenced memory error when asked to send a raw 0xF7

Reviewed-by: kizune

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 5 new commits pushed to the master branch:

  • 11431b1: 4619330: All built-in java.awt.color.ColorSpace fields should be specified as such
  • 17f04fc: 8254078: DataOutputStream is very slow post-disabling of Biased Locking
  • 79b7909: 8255980: G1 Service thread register_task can be used after shutdown
  • dd8e4ff: 8255711: Fix and unify hotspot signal handlers
  • d99e1f6: 8255991: Shenandoah: Apply 'weak' LRB on cmpxchg and xchg

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 openjdk bot added the ready Pull request is ready to be integrated label Nov 9, 2020
@mrserb
Copy link
Member Author

mrserb commented Nov 11, 2020

/integrate

@openjdk openjdk bot closed this Nov 11, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 11, 2020
@mrserb mrserb deleted the JDK-8237495 branch November 11, 2020 01:31
@openjdk
Copy link

openjdk bot commented Nov 11, 2020

@mrserb Since your change was applied there have been 31 commits pushed to the master branch:

  • be63525: 8211999: Window positioning bugs due to overlapping GraphicsDevice bounds (Windows/HiDPI)
  • 0a41ca6: 8254354: Add a withInvokeExactBehavior() VarHandle combinator
  • d6f1463: 8233332: Need to create exploded tests covering all forms of modules
  • f2a0bf3: 8256017: Remove unused elapsedTimer constructor
  • 7d4e86b: 8138588: VerifyMergedCPBytecodes option cleanup needed
  • a7f4691: 8244088: [Regression] Switch of Gnome theme ends up in deadlocked UI
  • bd3e65b: 8256052: Remove unused allocation type from fieldInfo
  • 6d8acd2: 8256066: Tests use deprecated TestNG API that is no longer available in new versions
  • 643969a: 8255822: Zero: improve build-time JVMTI handling
  • 6ae5e5b: 8221404: C2: Convert RegMask and IndexSet to use uintptr_t
  • ... and 21 more: https://git.openjdk.java.net/jdk/compare/c7551c37c7e9be7112371c351a4cc0d0d817cb46...master

Your commit was automatically rebased without conflicts.

Pushed as commit 5de99da.

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

openjdk-notifier bot referenced this pull request Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integrated Pull request has been integrated sound client-libs-dev@openjdk.org

Development

Successfully merging this pull request may close these issues.

2 participants