8287609: macOS: SIGSEGV at [CoreFoundation] CFArrayGetCount / sun.font.CFont.getTableBytesNative

[YaaZ]

CTFontCopyAvailableTables can return null, which causes subsequent call to CFArrayGetCount to crash with SEGFAULT, just added a null-check.

---

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-8287609: macOS: SIGSEGV at [CoreFoundation] CFArrayGetCount / sun.font.CFont.getTableBytesNative

Reviewers

• Phil Race (@prrace - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8962/head:pull/8962
$ git checkout pull/8962

Update a local copy of the PR:
$ git checkout pull/8962
$ git pull https://git.openjdk.java.net/jdk pull/8962/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8962

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8962.diff

[bridgekeeper]

👋 Welcome back YaaZ! 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]

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

• client

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.

[mlbridge]

Webrevs

• 00: Full (875437bb)

[prrace]

Where do you read it can return null ?

https://developer.apple.com/documentation/coretext/1510774-ctfontcopyavailabletables?language=objc

says nothing ..

[YaaZ]

I didn't find anything in documentation too, but it does return null, I checked with a debugger

[prrace]

All supported font formats on macOS have such tables, so it must be something else.
I don't see how it can fail unless there is something wrong with the CTFont that
would have been previously signalled or we have a bad font handle, or there is a bad table entry.
There's no way to tell from the stack trace.
So this fixes the symptom (temporarily, since it leaves the caller to deal with not being
able to get the tables it expects are there) but doesn't explain why it happens.

[YaaZ]

It looks for a "maxp" table which is absent for this font. If you look down the stack, it's trying to find units per em and if we return NULL, then it just falls back to upem=1000 (libharfbuzz/hb-ot-head-table.hh:53):

/* If no valid head table found, assume 1000, which matches typical Type1 usage. */
return 16 <= upem && upem <= 16384 ? upem : 1000;

And as I can see, nobody expects getTableBytesNative to always return a table, it's perfectly fine to return NULL when it's not found. The only question is why CTFontCopyAvailableTables returns NULL - font handle looks OK as you can see on the screenshot.

[prrace]

It looks for a "maxp" table which is absent for this font.

Are you sure ? I know that is what we are looking for but we call an API to get all tables without
specifying any particular table so maOS doesn't know ...

And that is a REQUIRED table per opentype + apple truetype
https://docs.microsoft.com/en-us/typography/opentype/spec/otff#font-tables
https://developer.apple.com/fonts/TrueType-Reference-Manual/RM06/Chap6.html
In shared code we throw an exception if we don't find it and you'd see this if you used the dynamic font loading APIs
and if it were installed on Windows or Linux we'd ignore the font.
I'm a bit surprised we get this far on mac without it - or that it isn't rejected by virtually every app.

Of course getTableBytesNative isn't guaranteed to return a table because you could ask for the 'abcd' table ..
But that macOS API didn't ask for a specific table, it just returned null for ALL tables because .... ?
Seems to me that there's at the least a macOS documentation deficiency but I'm not sure you've found the answer
to my question.

[YaaZ]

Sorry for the confusion, "maxp" being absent was only my assumption: CTFontCopyTable returned NULL for it, but I cannot be 100% sure about what's going on here, given that CTFontCopyAvailableTables doesn't even return anything.
BTW I see that CTFontCopyAvailableTables return type is marked as nullable, though conditions when it can be null are not specified.

So this is a very old font and I couldn't even find a tool which could edit it or at least parse and see what's inside, CoreText API is not very helpful as well. But the thing is: we can actually render this font, problems begins when we try to do a full layout, so in my opinion, adding this null-check is okay, we're just not doing anything useful in our layout code instead of crashing :).

And also if I'm not missing something, my change doesn't affect any behavior other that fixing the crash, look:

1. When I debugged it, I found around 5 or something calls to getTableBytesNative where it could crash, but the only table required by spec was "maxp" - I assume when asking for non-required table, callers already handle NULL case.
2. Caller uses "maxp" table to calculate upem (and falls back to 1000 if there's no such table) and set font scale (libharfbuzz/hb-font.cc:1512):

font->x_scale = font->y_scale = hb_face_get_upem (face);

3. But this scale is then overwritten anyway, so it doesn't even matter that we didn't find "maxp" table at all (libfontmanager/hb-jdk-font.cc:410):

hb_font_set_scale (font,
                  HBFloatToFixed(jdkFontInfo->ptSize*jdkFontInfo->devScale),
                  HBFloatToFixed(jdkFontInfo->ptSize*jdkFontInfo->devScale));

So it looks like there's really no behavior changed other than not crashing. In my tests text is properly rendered with Font.layoutGlyphVector being visually identical to Font.createGlyphVector.

[prrace]

oh my gosh .. it is a font designed for PRE-macOSX . meaning mac. so it is an ATM font with a resource fork and current macOS
still lets you install it which is a real surprise to me. That's why there are no tables.
OK I can see why this fix is needed

[openjdk]

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

8287609: macOS: SIGSEGV at [CoreFoundation] CFArrayGetCount / sun.font.CFont.getTableBytesNative

Reviewed-by: prr

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

• a113e16: 8287830: gtest fails to compile after JDK-8287661
• e4e1e8f: 8284199: Implementation of Structured Concurrency (Incubator)
• 308c068: 8287558: Remove remset coarsening stats during g1 remset summary printing
• d76c108: 8286772: java/awt/dnd/DropTargetInInternalFrameTest/DropTargetInInternalFrameTest.html times out and fails in Windows
• 005a330: 8287826: javax/accessibility/4702233/AccessiblePropertiesTest.java fails to compile
• e2cfe2e: 8231491: JDI tc02x004 failed again due to wrong # of breakpoints
• b6cdfd6: 8287740: NSAccessibilityShowMenuAction not working for text editors
• 25e9901: 8285305: Create an automated test for JDK-4495286
• 6f526e1: 8285373: Create an automated test for JDK-4702233
• a7e07fd: 8287102: ImageReaderSpi.canDecodeInput() for standard plugins should return false if a stream is too short
• ... and 56 more: https://git.openjdk.java.net/jdk/compare/8fc201e5bb7cb909a8bf496a751793b91b73631b...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@prrace) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[YaaZ]

/integrate

[openjdk]

@YaaZ
Your change (at version 875437b) is now ready to be sponsored by a Committer.

[YaaZ]

@prrace could you sponsor this, please? And also I believe this should be backported, but I don't know what the process is, where can I read about it?

[prrace]

/sponsor

[openjdk]

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

• 3df4b03: 8287837: ProblemList java/lang/ref/OOMEInReferenceHandler.java in -Xcomp
• a6fc485: 8287753: [spelling] close well-established compounds
• a113e16: 8287830: gtest fails to compile after JDK-8287661
• e4e1e8f: 8284199: Implementation of Structured Concurrency (Incubator)
• 308c068: 8287558: Remove remset coarsening stats during g1 remset summary printing
• d76c108: 8286772: java/awt/dnd/DropTargetInInternalFrameTest/DropTargetInInternalFrameTest.html times out and fails in Windows
• 005a330: 8287826: javax/accessibility/4702233/AccessiblePropertiesTest.java fails to compile
• e2cfe2e: 8231491: JDI tc02x004 failed again due to wrong # of breakpoints
• b6cdfd6: 8287740: NSAccessibilityShowMenuAction not working for text editors
• 25e9901: 8285305: Create an automated test for JDK-4495286
• ... and 58 more: https://git.openjdk.java.net/jdk/compare/8fc201e5bb7cb909a8bf496a751793b91b73631b...master

Your commit was automatically rebased without conflicts.

[openjdk]

@prrace @YaaZ Pushed as commit 8c460b0.

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

[prrace]

how do to backports is documented here : https://wiki.openjdk.java.net/display/SKARA/Backports
but the wiki is down for maintenance so you'll need to wait for it to be back up to read it.

[mlbridge]

Mailing list message from Justin Senseney on client-libs-dev:

Please remove me from your database

On Sat, Jun 4, 2022, 20:20 Nikita Gubarkov <duke at openjdk.java.net> wrote:

[mlbridge]

Mailing list message from Justin Senseney on client-libs-dev:

Please remove me from your mailing list

On Sat, Jun 4, 2022, 20:12 Phil Race <prr at openjdk.java.net> wrote:

[YaaZ]

Hi @prrace! Bot says I cannot use /backport command because it doesn't recognize me as contributor, though I'm /covered. Can you help me with this, please?

[aivanov-jdk]

Hi @prrace! Bot says I cannot use /backport command because it doesn't recognize me as contributor, though I'm /covered. Can you help me with this, please?

You can create a backport PR as regular one: create a branch in your fork of jdk18u, apply the changeset, commit and push the branch. Now create a PR with the title Backport <commit-hash> where commit-hash is the hash of the commit you're backporting, which is 8c460b043e1cbaf1f2d74958033bb24dea66a390 in this case.