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

Fix crashing google fonts or other TTF font files #121

Closed
wants to merge 1 commit into from

Conversation

wpmvangroesen
Copy link

We encountered a lot of issues with unpack errors in BinaryStream (see: #47 ) for some of the fonts hosted by Google.
The real fix would probably be to better understand the differences in the table definitions. We tried to check this using ttfdump or manually checking the font files, but we didn't find any big issues there. The codebase is pretty old and difficult to debug. So i don't know if anyone would be interested to investigate or fix this in a better way.

This commit solves the problem for us. The library is not crashing and fonts are being rendered in DOMPDF.

@Matergi
Copy link

Matergi commented Sep 15, 2023

this fixed the bug

@bsweeney bsweeney added this to the 0.5.5 milestone Sep 16, 2023
@daugaard47
Copy link

Please merge this commit.

@daugaard47
Copy link

If using Laravel here is how I'm adding this fix as a patch until this is merged. FYI, I'm using barryvdh/laravel-dompdf.

Step 1: Create a Patches Directory

  • In your Laravel project root, create a directory called patches.

Step 2: Create the Patch File

  1. Inside the patches directory, create a file named php-font-lib-readInt16-fix.patch.
  2. Open the file and paste the following patch content:
--- a/vendor/phenx/php-font-lib/src/FontLib/BinaryStream.php
+++ b/vendor/phenx/php-font-lib/src/FontLib/BinaryStream.php
@@ -147,9 +147,11 @@
  }

  public function readInt16() {
-    $a = unpack("nn", $this->read(2));
+    $data = $this->read(2);
+    if (!$data) return null;
+
+    $a = unpack("nn", $data);
     $v = $a["n"];
-
     if ($v >= 0x8000) {
       $v -= 0x10000;
     }

Step 3: Create the Patching Script

  1. Inside the patches directory, create a file named apply-patch.php.
  2. Open the file and paste the following PHP script:
<?php

$sourceFile = 'vendor/phenx/php-font-lib/src/FontLib/BinaryStream.php';
$backupFile = 'vendor/phenx/php-font-lib/src/FontLib/BinaryStream.php.bak';
$patchFile  = 'patches/php-font-lib-readInt16-fix.patch';

if (!file_exists($backupFile)) {
    copy($sourceFile, $backupFile);
}

exec("patch --forward -p1 < $patchFile 2>&1", $output, $returnVar);

if ($returnVar === 0) {
    echo "Patch applied successfully.\n";
} elseif (str_contains(implode("\n", $output), 'Skipping patch')) {
    echo "Patch was already applied. Skipping.\n";
} else {
    echo "Failed to apply patch.\n";
    exit(1);
}

Step 4: Update composer.json

  1. Open your composer.json file.
  2. Add the following lines to the scripts section:
"post-install-cmd": [
    "@php patches/apply-patch.php"
],
"post-update-cmd": [
    "@php patches/apply-patch.php"
],

Step 5: Apply the Patch

  • Run composer install or composer update to apply the patch.

This will apply the patch every time you run composer install or composer update, ensuring that the issue is fixed until this has been merged. Make sure to remove the patch after this packaged has been updated.

@wpmvangroesen
Copy link
Author

wpmvangroesen commented Sep 20, 2023

Nice comment for laravel users. You can also patch this with composer-patches (if using composer).

Add this to your composer.json:

{
   "require": {
    "cweagans/composer-patches": "^1.7"
  },
  "extra": {
    "enable-patching": "true",
    "patches": {
      "phenx/php-font-lib": {
        "Fix crashing google fonts":
          "https://github.com/beerntea/php-font-lib/commit/3b586cc9e78bc5415e32e6f5e42e88e500cfd3d2.patch"
      }
    }
  }
}

@hanifbd7
Copy link

hanifbd7 commented Oct 13, 2023

Hello wpmvangroesen
I did try with your code as bellow. But still getting this error "unpack(): Type n: not enough input, need 2, have 0"
My code runs in php 8.2+ and Laravel 10.

 "minimum-stability": "dev",
    "prefer-stable": true,
    "composer-patches":{
        "require": {
         "cweagans/composer-patches": "^1.7"
       },
       "extra": {
         "enable-patching": "true",
         "patches": {
           "phenx/php-font-lib": {
             "Fix crashing google fonts":
               "https://github.com/beerntea/php-font-lib/commit/3b586cc9e78bc5415e32e6f5e42e88e500cfd3d2.patch"
           }
         }
       }
     }

@wpmvangroesen
Copy link
Author

@hanifbd7 can you give some more details about the code? For ex what fonts are you using, what does your dompdf code look like, etc.

If we can reproduce it i can look into it!

@bsweeney
Copy link
Member

#128 addresses underlying causes of the unpack errors (at least, for those that have been identified). I'm not inclined to include this change since it just masks a parsing error in php-font-lib.

@bsweeney bsweeney closed this Oct 21, 2023
@wpmvangroesen
Copy link
Author

Of course! Thanks for looking into this and fixing it.
Do you have a timeline for the release of 0.5.5 ? :-)

@bsweeney
Copy link
Member

Current expectation is "as soon as possible" ... I'm trying to finish up the issues slated for the release. Only a few more issues left to investigate, and then some prep work for the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants