From 7f21e387874185c83af1a301e269946237b8e918 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 24 Jun 2018 09:53:32 +0200 Subject: [PATCH] Error, rather than warn, once a number of invalid path operators are encountered in `EvaluatorPreprocessor.read` (bug 1443140) Incomplete path operators, in particular, can result in fairly chaotic rendering artifacts, as can be observed on page four of the referenced PDF file. The initial (naive) solution that was attempted, was to simply throw a `FormatError` as soon as any invalid (i.e. too short) operator was found and rely on the existing `ignoreErrors` code-paths. However, doing so would have caused regressions in some files; see the existing `issue2391-1` test-case, which was promoted to an `eq` test to help prevent future bugs. Hence this patch, which adds special handling for invalid path operators since those may cause quite bad rendering artifacts. You could, in all fairness, argue that the patch is a handwavy solution and I wouldn't object. However, given that this only concerns *corrupt* PDF files, the way that PDF viewers (PDF.js included) try to gracefully deal with those could probably be described as a best-effort solution anyway. This patch also adjusts the existing `warn`/`info` messages to print the command name according to the PDF specification, rather than an internal PDF.js enumeration value. The former should be much more useful for debugging purposes. Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1443140. --- src/core/evaluator.js | 23 ++++++++++++++++++----- test/pdfs/bug1443140.pdf.link | 1 + test/test_manifest.json | 12 ++++++++++-- test/unit/evaluator_spec.js | 28 ++++++++++++++++++++++++++++ 4 files changed, 57 insertions(+), 7 deletions(-) create mode 100644 test/pdfs/bug1443140.pdf.link diff --git a/src/core/evaluator.js b/src/core/evaluator.js index 746b2ae3758ac..7f5ffd9dd7de9 100644 --- a/src/core/evaluator.js +++ b/src/core/evaluator.js @@ -2932,6 +2932,8 @@ var EvaluatorPreprocessor = (function EvaluatorPreprocessorClosure() { t['null'] = null; }); + const MAX_INVALID_PATH_OPS = 20; + function EvaluatorPreprocessor(stream, xref, stateManager) { this.opMap = getOPMap(); // TODO(mduan): pass array of knownCommands rather than this.opMap @@ -2939,6 +2941,7 @@ var EvaluatorPreprocessor = (function EvaluatorPreprocessorClosure() { this.parser = new Parser(new Lexer(stream, this.opMap), false, xref); this.stateManager = stateManager; this.nonProcessedArgs = []; + this._numInvalidPathOPS = 0; } EvaluatorPreprocessor.prototype = { @@ -2976,7 +2979,7 @@ var EvaluatorPreprocessor = (function EvaluatorPreprocessorClosure() { // Check that the command is valid var opSpec = this.opMap[cmd]; if (!opSpec) { - warn('Unknown command "' + cmd + '"'); + warn(`Unknown command "${cmd}".`); continue; } @@ -3002,18 +3005,28 @@ var EvaluatorPreprocessor = (function EvaluatorPreprocessorClosure() { } if (argsLength < numArgs) { + const partialMsg = `command ${cmd}: expected ${numArgs} args, ` + + `but received ${argsLength} args.`; + + // Incomplete path operators, in particular, can result in fairly + // chaotic rendering artifacts. Hence the following heuristics is + // used to error, rather than just warn, once a number of invalid + // path operators have been encountered (fixes bug1443140.pdf). + if ((fn >= OPS.moveTo && fn <= OPS.endPath) && // Path operator + ++this._numInvalidPathOPS > MAX_INVALID_PATH_OPS) { + throw new FormatError(`Invalid ${partialMsg}`); + } // If we receive too few arguments, it's not possible to execute // the command, hence we skip the command. - warn('Skipping command ' + fn + ': expected ' + numArgs + - ' args, but received ' + argsLength + ' args.'); + warn(`Skipping ${partialMsg}`); if (args !== null) { args.length = 0; } continue; } } else if (argsLength > numArgs) { - info('Command ' + fn + ': expected [0,' + numArgs + - '] args, but received ' + argsLength + ' args.'); + info(`Command ${cmd}: expected [0, ${numArgs}] args, ` + + `but received ${argsLength} args.`); } // TODO figure out how to type-check vararg functions diff --git a/test/pdfs/bug1443140.pdf.link b/test/pdfs/bug1443140.pdf.link new file mode 100644 index 0000000000000..426a447714fc4 --- /dev/null +++ b/test/pdfs/bug1443140.pdf.link @@ -0,0 +1 @@ +https://web.archive.org/web/20180324105403/https://engineering.purdue.edu/~chengkok/papers/2005/p507-li.pdf diff --git a/test/test_manifest.json b/test/test_manifest.json index e80912ad43c0f..b69054237f909 100644 --- a/test/test_manifest.json +++ b/test/test_manifest.json @@ -126,8 +126,7 @@ "file": "pdfs/issue2391-1.pdf", "md5": "25ae9cb959612e7b343b55da63af2716", "rounds": 1, - "lastPage": 1, - "type": "load" + "type": "eq" }, { "id": "issue2391-2", "file": "pdfs/issue2391-2.pdf", @@ -135,6 +134,15 @@ "rounds": 1, "type": "eq" }, + { "id": "bug1443140", + "file": "pdfs/bug1443140.pdf", + "md5": "8f9347b0d5620537850b24b8385b0982", + "rounds": 1, + "link": true, + "firstPage": 4, + "lastPage": 4, + "type": "eq" + }, { "id": "issue2531", "file": "pdfs/issue2531.pdf", "md5": "c58e6642d8a6e2ddd5e07a543ef8f30d", diff --git a/test/unit/evaluator_spec.js b/test/unit/evaluator_spec.js index 546ea74270e1b..d47429a300e8b 100644 --- a/test/unit/evaluator_spec.js +++ b/test/unit/evaluator_spec.js @@ -216,6 +216,34 @@ describe('evaluator', function() { done(); }); }); + + it('should error if (many) path operators have too few arguments ' + + '(bug 1443140)', function(done) { + const NUM_INVALID_OPS = 25; + const tempArr = new Array(NUM_INVALID_OPS + 1); + + // Non-path operators, should be ignored. + const invalidMoveText = tempArr.join('10 Td\n'); + const moveTextStream = new StringStream(invalidMoveText); + runOperatorListCheck(partialEvaluator, moveTextStream, + new ResourcesMock(), function(result) { + expect(result.argsArray).toEqual([]); + expect(result.fnArray).toEqual([]); + done(); + }); + + // Path operators, should throw error. + const invalidLineTo = tempArr.join('20 l\n'); + const lineToStream = new StringStream(invalidLineTo); + runOperatorListCheck(partialEvaluator, lineToStream, new ResourcesMock(), + function(error) { + expect(error instanceof FormatError).toEqual(true); + expect(error.message).toEqual( + 'Invalid command l: expected 2 args, but received 1 args.'); + done(); + }); + }); + it('should close opened saves', function(done) { var stream = new StringStream('qq'); runOperatorListCheck(partialEvaluator, stream, new ResourcesMock(),