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

8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars #249

Closed
wants to merge 8 commits into from
@@ -83,6 +83,7 @@
/* Miscellaneous (glib, fontconfig) */
static final native long g_utf8_offset_to_pointer(long str, long offset);
static final native long g_utf8_pointer_to_offset(long str, long pos);
static final native long g_utf8_strlen(long str, long max);
static final native long g_utf16_to_utf8(char[] str);
static final native void g_free(long ptr);
static final native int g_list_length(long list);
@@ -34,6 +34,10 @@
import com.sun.javafx.text.GlyphLayout;
import com.sun.javafx.text.TextRun;

import java.util.Arrays;
import java.util.LinkedHashMap;
import java.util.Map;

class PangoGlyphLayout extends GlyphLayout {
private static final long fontmap;

@@ -80,9 +84,8 @@ private boolean check(long checkValue, String message, long context, long desc,
return true;
}

private long str = 0L;
private Map<TextRun, Long> runUtf8 = new LinkedHashMap<>();
public void layout(TextRun run, PGFont font, FontStrike strike, char[] text) {

/* Create the pango font and attribute list */
FontResource fr = font.getFontResource();
boolean composite = fr instanceof CompositeFontResource;
@@ -126,17 +129,20 @@ public void layout(TextRun run, PGFont font, FontStrike strike, char[] text) {
OSPango.pango_attr_list_insert(attrList, attr);
}

if (str == 0L) {
str = OSPango.g_utf16_to_utf8(text);
Long str = runUtf8.get(run);
if (str == null) {
char[] rtext = Arrays.copyOfRange(text, run.getStart(), run.getEnd());
str = OSPango.g_utf16_to_utf8(rtext);
if (check(str, "Failed allocating UTF-8 buffer.", context, desc, attrList)) {
This conversation was marked as resolved by kevinrushforth

This comment has been minimized.

@prrace

prrace Jun 11, 2020 Collaborator

Did you really mean to store the run in the map before checking if str is null ?

This comment has been minimized.

@johanvos

johanvos Jun 12, 2020 Author Collaborator

I'm not sure what should happen if str is null. Do we want to be able to retry later? In that case, we shouldn't store it in the map. But if it is fatal, and seems to indicate a problem with rtext (e.g. too big?), so in that case it might not be worth retrying.

This comment has been minimized.

@kevinrushforth

kevinrushforth Jun 12, 2020 Member

If str is null, you would end up retrying it anyway, since Map::get will return null the next time (if there is a next time). Also, I think the dispose method might then crash trying to free a null pointer. So I think Phil is right and that you should not add the str to the Map if it is null.

This comment has been minimized.

@johanvos

johanvos Jun 12, 2020 Author Collaborator

I don't think it will be null but it will be 0 in which case it is stored in the Map
The only reason the str == null is then that we never tried to convert the Java chars to utf8, unless I'm missing a case?

This comment has been minimized.

@kevinrushforth

kevinrushforth Jun 12, 2020 Member

Oh, right. It will be a Long 0, not null. If you store it you will still have the problem I mentioned with dispose unless you add back in the str != 0 check. And you would need a check for str != 0 in the layout method so that the second time it doesn't treat it as a valid pointer. It might be safer to not store it if 0, which matches the current behavior?

This comment has been minimized.

@johanvos

johanvos Jun 12, 2020 Author Collaborator

That makes sense indeed. For the dispose call, a valid pointer is required.

This comment has been minimized.

@prrace

prrace Jun 12, 2020 Collaborator

I wasn't completely sure what the idea was, which is why I phrased it as a question :-)

return;
}
runUtf8.put(run, str);
}

/* Itemize */
long start = OSPango.g_utf8_offset_to_pointer(str, run.getStart());
long end = OSPango.g_utf8_offset_to_pointer(str, run.getEnd());
long runs = OSPango.pango_itemize(context, str, (int)(start - str), (int)(end - start), attrList, 0);
long utflen = OSPango.g_utf8_strlen(str,-1);
long end = OSPango.g_utf8_offset_to_pointer(str, utflen);
This conversation was marked as resolved by kevinrushforth

This comment has been minimized.

@kevinrushforth

kevinrushforth Jun 11, 2020 Member

Since you are now creating n native strings, 1 per substring based on the TextRun, rather than 1 for the entire String, isn't the start pointer wrong? Unless I am missing something, I would think that start should be set to str.

This comment has been minimized.

@kevinrushforth

kevinrushforth Jun 11, 2020 Member

I did a quick test, and setting start = str fixes the spurious assertions and intermittent crash.

This comment has been minimized.

@kevinrushforth

kevinrushforth Jun 11, 2020 Member

I should add that this is without any attempt to filter out 0 chars. Both UnicodeTextTest and UnicodeTextTest2 run correctly with no crashes and no assertions when I comment out the (ineffective) loop checking for 0 and set start = str leaving everything else as you have it in the current PR. Loading https://gluonhq.com/ in WebView works, too.

This comment has been minimized.

@prrace

prrace Jun 11, 2020 Collaborator

I agree. the text array is now the subset for the run, so effectively run.getStart() should be zero,
so there should be no need for that call. This probably looks OK when there is one run not otherwise.

This comment has been minimized.

@johanvos

johanvos Jun 12, 2020 Author Collaborator

You're both correct. I simplified it.

This comment has been minimized.

@prrace

prrace Jun 12, 2020 Collaborator

Looks good now.

long runs = OSPango.pango_itemize(context, str, 0, (int)(end - str), attrList, 0);

if (runs != 0) {
/* Shape all PangoItem into PangoGlyphString */
@@ -199,9 +205,9 @@ public void layout(TextRun run, PGFont font, FontStrike strike, char[] text) {
@Override
public void dispose() {
super.dispose();
if (str != 0L) {
for (Long str: runUtf8.values()) {
OSPango.g_free(str);
str = 0L;
}
runUtf8.clear();
}
}
@@ -398,6 +398,13 @@ JNIEXPORT jlong JNICALL OS_NATIVE(g_1utf8_1pointer_1to_1offset)
return (jlong)g_utf8_pointer_to_offset((const gchar *)str, (const gchar *)pos);
}

JNIEXPORT jlong JNICALL OS_NATIVE(g_1utf8_1strlen)
(JNIEnv *env, jclass that, jlong str, jlong pos)
{
if (!str) return 0;
return (jlong)g_utf8_strlen((const gchar *)str, (const gchar *)pos);
}

JNIEXPORT jlong JNICALL OS_NATIVE(g_1utf16_1to_1utf8)
(JNIEnv *env, jclass that, jcharArray str)
{
@@ -0,0 +1,130 @@
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation. Oracle designates this
* particular file as subject to the "Classpath" exception as provided
* by Oracle in the LICENSE file that accompanied this code.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

package test.com.sun.javafx.font.freetype;

import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import javafx.application.Application;
import javafx.application.Platform;
import javafx.scene.Scene;
import javafx.scene.layout.Pane;
import javafx.scene.layout.VBox;
import javafx.scene.text.Text;
import javafx.stage.Stage;
import javafx.stage.WindowEvent;

import org.junit.Test;
import org.junit.AfterClass;
import org.junit.BeforeClass;

import junit.framework.AssertionFailedError;
import static test.util.Util.TIMEOUT;

import static org.junit.Assert.*;

/**
* Test program for UTF16 to UTF8 conversion and Pango
*/
public class PangoTest {

static CountDownLatch launchLatch = new CountDownLatch(1);

static MyApp myApp;
static Pane pane;

public static class MyApp extends Application {

Stage stage = null;

public MyApp() {
super();
}

@Override
public void init() {
myApp = this;
}

@Override
public void start(Stage primaryStage) throws Exception {
this.stage = primaryStage;
pane = new VBox(10);
Scene scene = new Scene(pane, 400, 200);
stage.setScene(scene);
stage.addEventHandler(WindowEvent.WINDOW_SHOWN, e -> Platform.runLater(launchLatch::countDown));
stage.show();
}
}

@BeforeClass
public static void setupOnce() throws Exception {
// Start the Application
new Thread(() -> Application.launch(MyApp.class, (String[]) null)).start();
assertTrue("Timeout waiting for Application to launch",
launchLatch.await(TIMEOUT, TimeUnit.MILLISECONDS));
assertEquals(0, launchLatch.getCount());
}

@AfterClass
public static void teardownOnce() {
Platform.exit();
}

private void addTextToPane(Text text) throws Exception {
final CountDownLatch rDone = new CountDownLatch(1);
Platform.runLater(() -> {
text.layoutYProperty().addListener(inv -> {
rDone.countDown();
});
pane.getChildren().add(text);
});
assertTrue("Timeout waiting for runLater", rDone.await(TIMEOUT, TimeUnit.MILLISECONDS));
}

@Test
public void testZeroChar() throws Exception {
String FULL_UNICODE_SET;
StringBuilder builder = new StringBuilder();
for (int character = 0; character < 10000; character++) {
char[] chars = Character.toChars(character);
builder.append(chars);
}
FULL_UNICODE_SET = builder.toString();
Text text = new Text(FULL_UNICODE_SET);
addTextToPane(text);
}

@Test
public void testSurrogatePair() throws Exception {
StringBuilder builder = new StringBuilder();
builder.append(Character.toChars(55358));
builder.append(Character.toChars(56605));
builder.append(Character.toChars(8205));

Text text = new Text(builder.toString());
addTextToPane(text);
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.