Ensure that parseDefaultAppearance won't attempt to access a not yet defined variable (PR 12831 follow-up)
#12891
Conversation
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/2a62c33e33a8e79/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://3.101.106.178:8877/c3a93c3e6f263d3/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/2a62c33e33a8e79/output.txt Total script time: 26.73 mins
Image differences available at: http://54.67.70.0:8877/2a62c33e33a8e79/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://3.101.106.178:8877/c3a93c3e6f263d3/output.txt Total script time: 29.89 mins
Image differences available at: http://3.101.106.178:8877/c3a93c3e6f263d3/reftest-analyzer.html#web=eq.log |
…t 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.
…-tests The new names are not only less verbose, but also uses a *very* common PDF font-naming convention.
Snuffleupagus
force-pushed
the
DefaultAppearanceEvaluator-fixes
branch
from
f2e9c98
to
ef1d33a
Compare
|
/botio unittest |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://3.101.106.178:8877/8a3fb8fbe557051/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/8fd27f92a6bb8fb/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/8fd27f92a6bb8fb/output.txt Total script time: 3.63 mins
|
From: Bot.io (Windows)SuccessFull output at http://3.101.106.178:8877/8a3fb8fbe557051/output.txt Total script time: 4.68 mins
|
|
Thanks! |
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.lengthbefore attempting to read the next operator.Also, we can use the existing
EvaluatorPreprocessor.savedStatesDepthgetter to simplify the save/restore detection a tiny bit.[1] The ESLint rule
no-use-before-definewould 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.