From 6bcb4e3ad9d713649fc16f5e55b0362fcb41ad47 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 23 Jan 2021 02:58:36 +0100 Subject: [PATCH 1/2] Ensure that `parseDefaultAppearance` won't attempt to access a not yet defined variable (PR 12831 follow-up) Note how, in the `if (this.stateManager.stateStack.length !== 0) {` branch, we're attempting to access the not yet defined variable[1] `args`. If this code-path is ever hit, an Error will be thrown and parsing will thus be aborted immediately (likely leading to e.g. rendering bugs). Note that I found this purely by accident, since I happened to glance at the LGTM report. However, I've since found that the error is also present during the unit-test[2] and with this patch we're actually testing the *intended* thing here. As part of fixing this, and to avoid re-introducing a similar bug in the future, we'll now instead always reset `args.length` *before* attempting to read the next operator. Also, we can use the existing `EvaluatorPreprocessor.savedStatesDepth` getter to simplify the save/restore detection a tiny bit. --- [1] The ESLint rule `no-use-before-define` would have helped catch this problem, but unfortunately we cannot enable that without quite a bit of refactoring all over the code-base. [2] The unit-test was updated such that it would fail in the `master`-branch. --- src/core/default_appearance.js | 15 +++++++++------ test/unit/default_appearance_spec.js | 4 ++-- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/core/default_appearance.js b/src/core/default_appearance.js index 2b7eba5e69be9..7f55229dc726f 100644 --- a/src/core/default_appearance.js +++ b/src/core/default_appearance.js @@ -37,13 +37,17 @@ class DefaultAppearanceEvaluator extends EvaluatorPreprocessor { }; try { - while (this.read(operation)) { - if (this.stateManager.stateStack.length !== 0) { - // Don't get info in save/restore sections. - args.length = 0; - continue; + while (true) { + operation.args.length = 0; // Ensure that `args` it's always reset. + + if (!this.read(operation)) { + break; + } + if (this.savedStatesDepth !== 0) { + continue; // Don't get info in save/restore sections. } const { fn, args } = operation; + switch (fn | 0) { case OPS.setFont: const [fontName, fontSize] = args; @@ -64,7 +68,6 @@ class DefaultAppearanceEvaluator extends EvaluatorPreprocessor { ColorSpace.singletons.cmyk.getRgbItem(args, 0, result.fontColor, 0); break; } - args.length = 0; } } catch (reason) { warn(`parseDefaultAppearance - ignoring errors: "${reason}".`); diff --git a/test/unit/default_appearance_spec.js b/test/unit/default_appearance_spec.js index 53ba81f5e0def..36269c3c4700d 100644 --- a/test/unit/default_appearance_spec.js +++ b/test/unit/default_appearance_spec.js @@ -33,7 +33,7 @@ describe("Default appearance", function () { expect( parseDefaultAppearance( - " 0.1 0.2 0.3 rg /FontName 12 Tf 0.3 0.2 0.1 rg /NameFont 13 Tf" + "0.1 0.2 0.3 rg /FontName 12 Tf 0.3 0.2 0.1 rg /NameFont 13 Tf" ) ).toEqual({ fontSize: 13, @@ -44,7 +44,7 @@ describe("Default appearance", function () { it("should parse default appearance with save/restore", function () { const da = - "0.10 0.20 0.30 rg /FontName 12 Tf q 0.30 0.20 0.10 rg /NameFont 13 Tf Q"; + "q Q 0.10 0.20 0.30 rg /FontName 12 Tf q 0.30 0.20 0.10 rg /NameFont 13 Tf Q"; expect(parseDefaultAppearance(da)).toEqual({ fontSize: 12, fontName: Name.get("FontName"), From ef1d33a29e5903669ea85b77ab1f3221fb0c3d34 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 23 Jan 2021 03:54:10 +0100 Subject: [PATCH 2/2] Use slightly less verbose font-names in the "Default appearance" unit-tests The new names are not only less verbose, but also uses a *very* common PDF font-naming convention. --- test/unit/default_appearance_spec.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/unit/default_appearance_spec.js b/test/unit/default_appearance_spec.js index 36269c3c4700d..a1caf26da46b0 100644 --- a/test/unit/default_appearance_spec.js +++ b/test/unit/default_appearance_spec.js @@ -22,10 +22,10 @@ import { Name } from "../../src/core/primitives.js"; describe("Default appearance", function () { describe("parseDefaultAppearance and createDefaultAppearance", function () { it("should parse and create default appearance", function () { - const da = "/FontName 12 Tf 0.10 0.20 0.30 rg"; + const da = "/F1 12 Tf 0.10 0.20 0.30 rg"; const result = { fontSize: 12, - fontName: Name.get("FontName"), + fontName: Name.get("F1"), fontColor: new Uint8ClampedArray([26, 51, 76]), }; expect(parseDefaultAppearance(da)).toEqual(result); @@ -33,21 +33,21 @@ describe("Default appearance", function () { expect( parseDefaultAppearance( - "0.1 0.2 0.3 rg /FontName 12 Tf 0.3 0.2 0.1 rg /NameFont 13 Tf" + "0.1 0.2 0.3 rg /F1 12 Tf 0.3 0.2 0.1 rg /F2 13 Tf" ) ).toEqual({ fontSize: 13, - fontName: Name.get("NameFont"), + fontName: Name.get("F2"), fontColor: new Uint8ClampedArray([76, 51, 26]), }); }); it("should parse default appearance with save/restore", function () { const da = - "q Q 0.10 0.20 0.30 rg /FontName 12 Tf q 0.30 0.20 0.10 rg /NameFont 13 Tf Q"; + "q Q 0.10 0.20 0.30 rg /F1 12 Tf q 0.30 0.20 0.10 rg /F2 13 Tf Q"; expect(parseDefaultAppearance(da)).toEqual({ fontSize: 12, - fontName: Name.get("FontName"), + fontName: Name.get("F1"), fontColor: new Uint8ClampedArray([26, 51, 76]), }); });