Pin the bundled font paths so PDF export is not blank (#371, #251, #287)

.github/workflows/integration.yml

@@ -85,6 +85,8 @@ jobs:
       - name: install what drives the browser
         run: |
           python3 -m pip install --quiet websockets requests
+          # pdftotext, for reading the text back out of a converted PDF
+          sudo apt-get install -y -qq poppler-utils
           # the runner image ships Chrome; the rig will take either
           echo "RIG_CHROMIUM=$(command -v google-chrome || command -v chromium || command -v chromium-browser)" >> "$GITHUB_ENV"
 

.github/workflows/lint.yml

@@ -35,7 +35,7 @@ jobs:
 
       - name: lint
         run: |
-          find lib appinfo tests -name '*.php' -not -path './3rdparty/*' -print0 \
+          find lib appinfo tests tools -name '*.php' -not -path './3rdparty/*' -print0 \
             | xargs -0 -n1 -P4 php -l
 
   javascript:

.nextcloudignore

@@ -10,3 +10,4 @@ screenshots
 .nextcloudignore
 Makefile
 tests
+tools

Makefile

@@ -46,6 +46,7 @@ appstore:
 	--exclude="../$(app_name)/3rdparty/onlyoffice/documentserver/fonts" \
 	--exclude="../$(app_name)/build" \
 	--exclude="../$(app_name)/tests" \
+	--exclude="../$(app_name)/tools" \
 	--exclude="../$(app_name)/vendor" \
 	--exclude="../$(app_name)/composer.json" \
 	--exclude="../$(app_name)/composer.lock" \
@@ -141,13 +142,23 @@ appstore:
 	# Pin the generated font paths to x2t's working directory
 	# (server/FileConverter/bin). allfontsgen writes them out prefixed with its
 	# own working directory, which is neither where x2t runs nor the same path
-	# on the machine that installs the app; x2t then hands a null buffer to
-	# CFontFileLoader.LoadFontFromData and any change replay that has to measure
-	# text - saving a spreadsheet, for one - dies with "Cannot read property
-	# 'length' of null". The paths in font_selection.bin alongside it are not
-	# used for loading, so they can stay as generated.
-	sed -i 's|"[^"]*/core-fonts/|"../../../core-fonts/|g' \
-		3rdparty/onlyoffice/documentserver/server/FileConverter/bin/AllFonts.js
+	# on the machine that installs the app - and this tarball ships to every
+	# installation, so what is generated here is what they all get.
+	#
+	# Both of the files it writes have to be pinned. sdkjs reads AllFonts.js
+	# during change replay and hands the null buffer to
+	# CFontFileLoader.LoadFontFromData, so saving a spreadsheet dies with
+	# "Cannot read property 'length' of null"; x2t's PDF writer reads
+	# font_selection.bin and, when the path does not resolve, writes a PDF with
+	# no glyphs in it and exits 0 (#371, #251, #287). This used to pin only
+	# AllFonts.js, on the belief that font_selection.bin was not used for
+	# loading, which is why PDF export was blank for four years.
+	#
+	# font_selection.bin is not sed-able - its strings are length-prefixed -
+	# hence the php, which shares its rewriter with the runtime rebuild.
+	command -v php >/dev/null || { echo 'php is needed to pin the font paths' >&2; exit 1; }
+	php tools/pin-font-paths.php \
+		3rdparty/onlyoffice/documentserver/server/FileConverter/bin
 	# Build the presentation design themes. Another job the DocService we do not
 	# ship normally does: the package only carries their sources under
 	# sdkjs/slide/themes/src, so without this the presentation editor's design

README.md

@@ -49,6 +49,12 @@ You can add custom fonts to the document server using the following occ commands
 - Add font by path `occ documentserver:fonts --add /usr/share/fonts/myfont.ttf`
 - List added fonts `occ documentserver:fonts`
 - Remove an added font `occ documentserver:fonts --remove myfont.ttf`
+- Rebuild the font library `occ documentserver:fonts --rebuild`
+
+Each of these rebuilds the document server's font library, which needs the app
+directory to be writable by the web server user. If it is not, the command says
+so and exits non-zero — the bundled fonts keep working, but fonts you add will
+not be picked up.
 
 ## Self signed certificates
 

lib/Command/Fonts.php

@@ -77,8 +77,11 @@ protected function execute(InputInterface $input, OutputInterface $output) {
 		} catch (\Exception $e) {
 			$error = $e->getMessage();
 			$output->writeln("<error>$error</error>");
-			return 0;
+			// These two were the wrong way round, so a rebuild that failed
+			// exited 0 and one that worked exited 1: anything scripting this
+			// was told the opposite of what happened.
+			return 1;
 		}
-		return 1;
+		return 0;
 	}
 }

lib/Document/ConverterBinary.php

@@ -28,6 +28,9 @@
 class ConverterBinary {
 	public const BINARY_DIRECTORY = __DIR__ . '/../../3rdparty/onlyoffice/documentserver/server/FileConverter/bin';
 
+	/** What x2t prints on stdout, and only on stdout, for a font it cannot open. */
+	private const MISSING_FONT = "Can't load fontfile ";
+
 	private $logger;
 
 	public function __construct(LoggerInterface $logger) {
@@ -84,9 +87,42 @@ public function run(string $param, ?string $password = null): string {
 			throw new DocumentConversionException("x2t exited with status $status");
 		}
 
+		$this->reportMissingFonts($output);
+
 		return $output;
 	}
 
+	/**
+	 * A font x2t could not load is a line on stdout and nothing else.
+	 *
+	 * It carries on and exits 0, and what comes out is a document with no
+	 * glyphs in it - the whole of #371, #251 and #287, which went four years
+	 * without a diagnostic because a blank PDF and a good one are the same
+	 * successful conversion from here. Not fatal: one missing custom font
+	 * should not fail a conversion that is otherwise fine. But it goes in the
+	 * log, because nothing else in the system can tell the difference.
+	 */
+	private function reportMissingFonts(string $output): void {
+		if (strpos($output, self::MISSING_FONT) === false) {
+			return;
+		}
+
+		$missing = [];
+		foreach (explode("\n", $output) as $line) {
+			$at = strpos($line, self::MISSING_FONT);
+			if ($at !== false) {
+				$missing[trim(substr($line, $at + strlen(self::MISSING_FONT)))] = true;
+			}
+		}
+
+		$this->logger->error(
+			'x2t could not load ' . count($missing) . ' font(s); anything set in them converts '
+			. 'with no glyphs at all. Run `occ documentserver:fonts --rebuild`. Missing: '
+			. implode(', ', array_slice(array_keys($missing), 0, 5)),
+			['app' => 'documentserver_community']
+		);
+	}
+
 	public function test(): bool {
 		try {
 			$output = $this->run('');

lib/Document/FontManager.php

@@ -56,9 +56,39 @@ public function rebuildFonts() {
 
 			// Absolute paths throughout: since 9.x allfontsgen no longer
 			// resolves relative arguments against the working directory, and
-			// with relative ones it just writes out an empty font list.
-			$binDir = ConverterBinary::BINARY_DIRECTORY;
-			$documentServer = $binDir . '/../../..';
+			// with relative ones it just writes out an empty font list. And
+			// resolved rather than left with the '/../..' this file reaches
+			// them by, so that what ends up in the font lists, and in the
+			// errors below, is a path an admin can act on.
+			$binDir = realpath(ConverterBinary::BINARY_DIRECTORY) ?: ConverterBinary::BINARY_DIRECTORY;
+			$documentServer = realpath($binDir . '/../../..') ?: $binDir . '/../../..';
+
+			// Before anything runs: a rebuild that cannot write its output is
+			// the failure this whole method exists to make visible, and
+			// allfontsgen signals it by exiting 0 with nothing on either pipe.
+			// Saying so here names the file, which "no usable font list" below
+			// cannot.
+			$this->assertWritable([
+				$binDir . '/AllFonts.js',
+				$binDir . '/font_selection.bin',
+				$documentServer . '/sdkjs/common/AllFonts.js',
+				$documentServer . '/fonts',
+			]);
+
+			// How a run that wrote nothing at all is told apart from one that
+			// rewrote both files with identical content, which is the ordinary
+			// case: allfontsgen rewrites them either way, so the mtime moves
+			// either way. Two readings because neither is enough on its own -
+			// a second rebuild inside the same second leaves the mtime where it
+			// was, and a filesystem clock that runs behind this one (NFS) never
+			// reaches $startedAt.
+			$startedAt = time();
+			$before = [];
+			foreach ([$binDir . '/AllFonts.js', $binDir . '/font_selection.bin'] as $path) {
+				clearstatcache(true, $path);
+				$before[$path] = @filemtime($path);
+			}
+
 			$cmd = $binDir . '/../../tools/allfontsgen \
 				--input="' . $documentServer . '/core-fonts" \
 				--input="' . $fontsDir . '" \
@@ -95,37 +125,115 @@ public function rebuildFonts() {
 				throw new \Exception($error);
 			}
 
-			$this->finishFontList($binDir . '/AllFonts.js', $status, $output);
+			$this->finishFontList($binDir, $status, $output, $startedAt, $before);
 		});
 	}
 
 	/**
-	 * Check what allfontsgen produced, then pin the bundled font paths to
-	 * x2t's working directory (server/FileConverter/bin).
+	 * Fail unless every path is writable, or can be created.
+	 *
+	 * @param string[] $paths
+	 */
+	private function assertWritable(array $paths): void {
+		foreach ($paths as $path) {
+			$target = file_exists($path) ? $path : dirname($path);
+			if (!is_writable($target)) {
+				$user = function_exists('posix_geteuid')
+					? (posix_getpwuid(posix_geteuid())['name'] ?? (string)posix_geteuid())
+					: 'the web server user';
+				throw new \Exception(
+					"Cannot rebuild the font library: $target is not writable by $user. "
+					. "Custom fonts will not be picked up, and on an install made before the "
+					. "bundled font paths were pinned at build time, PDF export stays blank."
+				);
+			}
+		}
+	}
+
+	/**
+	 * Check that this run of allfontsgen produced both font lists, then pin the
+	 * bundled font paths in them to x2t's working directory
+	 * (server/FileConverter/bin).
 	 *
-	 * Two problems are handled here, both of which allfontsgen reports by
-	 * exiting successfully with nothing on stderr. It writes no font list at
-	 * all when it cannot create the file, and it writes an empty one when it
-	 * cannot find the fonts. It also prefixes every path it does write with its
-	 * own working directory, which stops resolving as soon as the app directory
-	 * moves. In each case x2t ends up handing a null buffer to
-	 * CFontFileLoader.LoadFontFromData, and any change replay that has to
+	 * Everything allfontsgen gets wrong here it reports by exiting successfully
+	 * with nothing on stderr: it writes no font list at all when it cannot
+	 * create the file, an empty one when it cannot find the fonts, and it
+	 * prefixes every path it does write with its own working directory, which
+	 * stops resolving as soon as the app directory moves - or, for an app store
+	 * install, the moment it leaves the machine that built it.
+	 *
+	 * Nothing downstream notices any of that. sdkjs hands the null buffer to
+	 * CFontFileLoader.LoadFontFromData and any change replay that has to
 	 * measure text - saving a spreadsheet, for one - fails with "Cannot read
-	 * property 'length' of null". Paths to custom fonts live outside the app
-	 * directory and stay absolute.
+	 * property 'length' of null"; x2t's PDF writer prints "Can't load fontfile"
+	 * on a stream nobody reads and writes a PDF with no glyphs in it (#371,
+	 * #251, #287). So the check has to happen here, and it has to be loud: the
+	 * repair step turns it into a warning at `occ upgrade`, which is four years
+	 * earlier than the alternative.
+	 *
+	 * Paths to custom fonts live outside the app directory and stay absolute.
 	 */
-	private function finishFontList(string $allFontsJs, int $status, string $output): void {
+	private function finishFontList(string $binDir, int $status, string $output, int $startedAt, array $before): void {
+		$allFontsJs = $binDir . '/AllFonts.js';
+		$selection = $binDir . '/font_selection.bin';
+
+		foreach ([$allFontsJs, $selection] as $path) {
+			clearstatcache(true, $path);
+			$mtime = @filemtime($path);
+
+			if ($mtime === false || @filesize($path) === 0) {
+				throw new \Exception(
+					"allfontsgen wrote no $path (exit status $status) " . trim($output)
+				);
+			}
+			// A stale file left over from the build satisfies every check on
+			// its contents, so the one thing worth knowing about it is that
+			// this run did not write it.
+			if ($mtime === $before[$path] && $mtime < $startedAt) {
+				throw new \Exception(
+					"allfontsgen left $path untouched (exit status $status), so the font "
+					. "paths in it are still the ones it was built with " . trim($output)
+				);
+			}
+		}
+
 		$content = @file_get_contents($allFontsJs);
 		if ($content === false || strpos($content, '/core-fonts/') === false) {
 			throw new \Exception(
 				"allfontsgen produced no usable font list in $allFontsJs (exit status $status) "
 				. trim($output)
 			);
 		}
+		$this->write($allFontsJs, FontPaths::pinAllFontsJs($content));
+
+		$data = @file_get_contents($selection);
+		if ($data === false) {
+			throw new \Exception("Cannot read $selection");
+		}
+		$pinned = FontPaths::pinFontSelection($data);
+		$this->write($selection, $pinned);
+
+		// The pinning is the part with nothing else watching it: a path form
+		// the rewriter does not recognise would leave x2t loading no fonts
+		// exactly as before, and exiting 0 about it.
+		$paths = FontPaths::fontSelectionPaths($pinned);
+		if (!$paths) {
+			throw new \Exception(
+				"No bundled font paths in $selection (exit status $status) " . trim($output)
+			);
+		}
+		foreach ($paths as $path) {
+			if (!is_file($binDir . '/' . $path)) {
+				throw new \Exception(
+					"Font path in $selection does not resolve from $binDir: $path"
+				);
+			}
+		}
+	}
 
-		$pinned = preg_replace('|"[^"]*/core-fonts/|', '"../../../core-fonts/', $content);
-		if ($pinned !== null && $pinned !== $content) {
-			file_put_contents($allFontsJs, $pinned);
+	private function write(string $path, string $content): void {
+		if (file_put_contents($path, $content) === false) {
+			throw new \Exception("Cannot write $path");
 		}
 	}