Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8189092: ArrayIndexOutOfBoundsException on Linux in getCachedGlyph #24

Closed
wants to merge 2 commits into from

Conversation

@prrace
Copy link
Contributor

prrace commented Oct 25, 2019

I have added an evaluation in https://bugs.openjdk.java.net/browse/JDK-8207839
Please read that for more detail, but basically we are not properly preventing or
handling cases where fontconfig causes us to overflow the byte storage used
for a font "slot".

Progress

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

Issue

JDK-8189092: ArrayIndexOutOfBoundsException on Linux in getCachedGlyph

Approvers

  • Kevin Rushforth (kcr - Reviewer)
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 25, 2019

👋 Welcome back prr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

@openjdk openjdk bot added the rfr label Oct 25, 2019
@mlbridge
Copy link

mlbridge bot commented Oct 25, 2019

Webrevs

Copy link
Member

kevinrushforth left a comment

Looks good to me. I don't think this needs a second review, although @bourgesl or @johanvos might want to weigh in.

@openjdk openjdk bot removed the rfr label Oct 25, 2019
@bourgesl
Copy link

bourgesl commented Oct 26, 2019

I had a quick look by curiosity as it is not my field. Explanations really make sense, thanks Phil.
Bit masks & shifts look good and definitely it will make that code safer.
However I wonder how to deal with more than 255 fonts (1 byte limit) as it may happen on some systems, even not reallistic.
Thanks

@kevinrushforth kevinrushforth changed the title 8189092: ArrayIndexOutOfBoundsException on Linux in getCachedGlyph WIP: 8189092: ArrayIndexOutOfBoundsException on Linux in getCachedGlyph Oct 26, 2019
@kevinrushforth kevinrushforth changed the title WIP: 8189092: ArrayIndexOutOfBoundsException on Linux in getCachedGlyph 8189092: ArrayIndexOutOfBoundsException on Linux in getCachedGlyph Oct 26, 2019
@openjdk
Copy link

openjdk bot commented Oct 26, 2019

@prrace This change can now be integrated. The commit message will be:

8189092: ArrayIndexOutOfBoundsException on Linux in getCachedGlyph

Reviewed-by: kcr
  • If you would like to add a summary, use the /summary command.
  • To list additional contributors, use the /contributor command.

Since the source branch of this PR was last updated there have been 2 commits pushed to the master branch:

  • ac71396: 8232929: Duplicate symbols when building static libraries
  • ab6ea3b: 8232158: [macOS] Fallback to command line tools if xcode is missing

Since there are no conflicts, your changes will automatically be rebased on top of the above commits when integrating. If you prefer to do this manually, please merge master into your branch first.

  • To integrate this PR with the above commit message, type /integrate in a new comment.
@openjdk openjdk bot added the ready label Oct 26, 2019
@prrace
Copy link
Contributor Author

prrace commented Oct 26, 2019

However I wonder how to deal with more than 255 fonts (1 byte limit) as it may happen on some systems, even not reallistic.

That would have to be > 255 fonts per fontconfig font, and we have additional filters
to eliminate fonts that don't add value (at least we do now with fixing the bug in that
code in fontpath_linux.c).
We could rework things to use 16 bit for font slot, although that would be a bigger change
and require more testing and really this is never going to be needed on Windows or Mac,
just some very small number of Linux systems that are not default configs ..

@prrace
Copy link
Contributor Author

prrace commented Oct 26, 2019

/integrate

@openjdk openjdk bot closed this Oct 26, 2019
@openjdk openjdk bot added the integrated label Oct 26, 2019
@openjdk
Copy link

openjdk bot commented Oct 26, 2019

@prrace The following commits have been pushed to master since your change was applied:

  • ac71396: 8232929: Duplicate symbols when building static libraries
  • ab6ea3b: 8232158: [macOS] Fallback to command line tools if xcode is missing

Your commit was automatically rebased without conflicts.

Pushed as commit 5a70b0c.

@openjdk openjdk bot removed the ready label Oct 26, 2019
@mlbridge
Copy link

mlbridge bot commented Oct 26, 2019

Mailing list message from Phil Race prr@openjdk.org

Changeset: 5a70b0c
Author: Phil Race
Date: 2019-10-26 15:14:47 +0000
URL: https://git.openjdk.java.net/jfx/commit/5a70b0c5

8189092: ArrayIndexOutOfBoundsException on Linux in getCachedGlyph

Reviewed-by: kcr

! modules/javafx.graphics/src/main/java/com/sun/javafx/font/CompositeGlyphMapper.java
! modules/javafx.graphics/src/main/java/com/sun/prism/impl/GlyphCache.java
! modules/javafx.graphics/src/main/native-font/fontpath_linux.c

@prrace prrace deleted the prrace:8189092 branch Oct 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.