Sakurai/pdf.js
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Ensure that parseDefaultAppearance won't attempt to access a not yet defined variable (PR 12831 follow-up)

Browse Source 
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.
This commit is contained in:
Jonas Jenwald 2021-01-23 02:58:36 +01:00
parent 6ffb6b1c0c
commit 6bcb4e3ad9
2 changed files with 11 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
src/core/default_appearance.js
View File

@ -37,13 +37,17 @@ class DefaultAppearanceEvaluator extends EvaluatorPreprocessor {
}; };
try { try {
while (this.read(operation)) { while (true) {
if (this.stateManager.stateStack.length !== 0) { operation.args.length = 0; // Ensure that `args` it's always reset.
// Don't get info in save/restore sections.
args.length = 0; if (!this.read(operation)) {
continue; break;
}
if (this.savedStatesDepth !== 0) {
continue; // Don't get info in save/restore sections.
} }
const { fn, args } = operation; const { fn, args } = operation;
switch (fn | 0) { switch (fn | 0) {
case OPS.setFont: case OPS.setFont:
const [fontName, fontSize] = args; const [fontName, fontSize] = args;
@ -64,7 +68,6 @@ class DefaultAppearanceEvaluator extends EvaluatorPreprocessor {
ColorSpace.singletons.cmyk.getRgbItem(args, 0, result.fontColor, 0); ColorSpace.singletons.cmyk.getRgbItem(args, 0, result.fontColor, 0);
break; break;
} }
args.length = 0;
} }
} catch (reason) { } catch (reason) {
warn(`parseDefaultAppearance - ignoring errors: "${reason}".`); warn(`parseDefaultAppearance - ignoring errors: "${reason}".`);

4
test/unit/default_appearance_spec.js
View File

@ -33,7 +33,7 @@ describe("Default appearance", function () {
expect( expect(
parseDefaultAppearance( 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({ ).toEqual({
fontSize: 13, fontSize: 13,
@ -44,7 +44,7 @@ describe("Default appearance", function () {
it("should parse default appearance with save/restore", function () { it("should parse default appearance with save/restore", function () {
const da = 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({ expect(parseDefaultAppearance(da)).toEqual({
fontSize: 12, fontSize: 12,
fontName: Name.get("FontName"), fontName: Name.get("FontName"),