From f7b90ef253ef928ba32d7aed75556b384738f025 Mon Sep 17 00:00:00 2001 From: Chris Liddell Date: Wed, 5 Sep 2018 17:14:59 +0100 Subject: [PATCH 1/2] Bug 699718: Ensure stack space is available before gsrestore call out During a grestore, if the device is going to change, we call out to Postscript to restore the device configuration, before returning to restore the graphics state internally. We have to ensure sufficient op stack space is available to complete the operation, otherwise the device can end up an undefined state. --- Resource/Init/gs_setpd.ps | 23 +++++++++++++------- psi/zdevice2.c | 55 ++++++++++++++++++++++++++++++++++++----------- 2 files changed, 58 insertions(+), 20 deletions(-) diff --git a/Resource/Init/gs_setpd.ps b/Resource/Init/gs_setpd.ps index aee9614..8fa7c51 100644 --- a/Resource/Init/gs_setpd.ps +++ b/Resource/Init/gs_setpd.ps @@ -96,7 +96,7 @@ level2dict begin % we must (carefully) reinstall the old parameters in % the same device. .currentpagedevice pop //null currentdevice //null - {.trysetparams} .internalstopped + { .trysetparams } .internalstopped { //null } if @@ -104,25 +104,32 @@ level2dict begin { pop pop } { SETPDDEBUG { (Error in .trysetparams!) = pstack flush } if - cleartomark pop pop pop - % if resetting the entire device state failed, at least put back the se - currentdevice //null //false mark /.LockSafetyParams .currentpagedevice pop - /.LockSafetyParams .knownget not {//false} if .putdeviceparamsonly + {cleartomark pop pop pop} .internalstopped pop + % if resetting the entire device state failed, at least put back the + % security related key + currentdevice //null //false mark /.LockSafetyParams + currentpagedevice /.LockSafetyParams .knownget not + {systemdict /SAFER .knownget not {//false} } if + .putdeviceparamsonly /.installpagedevice cvx /rangecheck signalerror } ifelse pop pop % A careful reading of the Red Book reveals that an erasepage % should occur, but *not* an initgraphics. erasepage .beginpage - } bind def + } bind executeonly def /.uninstallpagedevice - { 2 .endpage { .currentnumcopies //false .outputpage } if + { + {2 .endpage { .currentnumcopies //false .outputpage } if} .internalstopped pop nulldevice } bind def (%grestorepagedevice) cvn - { .uninstallpagedevice grestore .installpagedevice + { + .uninstallpagedevice + grestore + .installpagedevice } bind def (%grestoreallpagedevice) cvn diff --git a/psi/zdevice2.c b/psi/zdevice2.c index 0c7080d..5447c8c 100644 --- a/psi/zdevice2.c +++ b/psi/zdevice2.c @@ -251,8 +251,8 @@ z2currentgstate(i_ctx_t *i_ctx_p) /* ------ Wrappers for operators that reset the graphics state. ------ */ /* Check whether we need to call out to restore the page device. */ -static bool -restore_page_device(const gs_gstate * pgs_old, const gs_gstate * pgs_new) +static int +restore_page_device(i_ctx_t *i_ctx_p, const gs_gstate * pgs_old, const gs_gstate * pgs_new) { gx_device *dev_old = gs_currentdevice(pgs_old); gx_device *dev_new; @@ -260,9 +260,10 @@ restore_page_device(const gs_gstate * pgs_old, const gs_gstate * pgs_new) gx_device *dev_t2; bool samepagedevice = obj_eq(dev_old->memory, &gs_int_gstate(pgs_old)->pagedevice, &gs_int_gstate(pgs_new)->pagedevice); + bool LockSafetyParams = dev_old->LockSafetyParams; if ((dev_t1 = (*dev_proc(dev_old, get_page_device)) (dev_old)) == 0) - return false; + return 0; /* If we are going to putdeviceparams in a callout, we need to */ /* unlock temporarily. The device will be re-locked as needed */ /* by putdeviceparams from the pgs_old->pagedevice dict state. */ @@ -271,23 +272,44 @@ restore_page_device(const gs_gstate * pgs_old, const gs_gstate * pgs_new) dev_new = gs_currentdevice(pgs_new); if (dev_old != dev_new) { if ((dev_t2 = (*dev_proc(dev_new, get_page_device)) (dev_new)) == 0) - return false; - if (dev_t1 != dev_t2) - return true; + samepagedevice = true; + else if (dev_t1 != dev_t2) + samepagedevice = false; + } + + if (LockSafetyParams && !samepagedevice) { + os_ptr op = osp; + const int max_ops = 512; + + /* The %grestorepagedevice must complete: the biggest danger + is operand stack overflow. As we use get/putdeviceparams + that means pushing all the device params onto the stack, + pdfwrite having by far the largest number of parameters + at (currently) 212 key/value pairs - thus needing (currently) + 424 entries on the op stack. Allowing for working stack + space, and safety margin..... + */ + if (max_ops > op - osbot) { + if (max_ops >= ref_stack_count(&o_stack)) + return_error(gs_error_stackoverflow); + } } /* * The current implementation of setpagedevice just sets new * parameters in the same device object, so we have to check * whether the page device dictionaries are the same. */ - return !samepagedevice; + return samepagedevice ? 0 : 1; } /* - grestore - */ static int z2grestore(i_ctx_t *i_ctx_p) { - if (!restore_page_device(igs, gs_gstate_saved(igs))) + int code = restore_page_device(i_ctx_p, igs, gs_gstate_saved(igs)); + if (code < 0) return code; + + if (code == 0) return gs_grestore(igs); return push_callout(i_ctx_p, "%grestorepagedevice"); } @@ -297,7 +319,9 @@ static int z2grestoreall(i_ctx_t *i_ctx_p) { for (;;) { - if (!restore_page_device(igs, gs_gstate_saved(igs))) { + int code = restore_page_device(i_ctx_p, igs, gs_gstate_saved(igs)); + if (code < 0) return code; + if (code == 0) { bool done = !gs_gstate_saved(gs_gstate_saved(igs)); gs_grestore(igs); @@ -328,11 +352,15 @@ z2restore(i_ctx_t *i_ctx_p) if (code < 0) return code; while (gs_gstate_saved(gs_gstate_saved(igs))) { - if (restore_page_device(igs, gs_gstate_saved(igs))) + code = restore_page_device(i_ctx_p, igs, gs_gstate_saved(igs)); + if (code < 0) return code; + if (code > 0) return push_callout(i_ctx_p, "%restore1pagedevice"); gs_grestore(igs); } - if (restore_page_device(igs, gs_gstate_saved(igs))) + code = restore_page_device(i_ctx_p, igs, gs_gstate_saved(igs)); + if (code < 0) return code; + if (code > 0) return push_callout(i_ctx_p, "%restorepagedevice"); code = dorestore(i_ctx_p, asave); @@ -355,9 +383,12 @@ static int z2setgstate(i_ctx_t *i_ctx_p) { os_ptr op = osp; + int code; check_stype(*op, st_igstate_obj); - if (!restore_page_device(igs, igstate_ptr(op))) + code = restore_page_device(i_ctx_p, igs, igstate_ptr(op)); + if (code < 0) return code; + if (code == 0) return zsetgstate(i_ctx_p); return push_callout(i_ctx_p, "%setgstatepagedevice"); } -- 2.14.4 From 9a1f4832bc862c2a449cd90c46b1ccc61a829d48 Mon Sep 17 00:00:00 2001 From: Chris Liddell Date: Fri, 7 Sep 2018 08:07:12 +0100 Subject: [PATCH 2/2] Bug 699718(2): Improve/augment stack size checking Improve the rebustness of the previous solution (previously it could trigger an error when there *was* stack capacity available). Remove redundant check: we don't need to check if the *current* stack size is sufficient, before checking the maximum permitted stack size. Also check the exec stack, as execstackoverflow can also cause the Postscript call out to fail. Lastly, in event of failure, put the LockSafetyParams flag back in the existing device (this is only necessary because we don't enfore JOBSERVER mode). Note: the Postscript callout (%grestorepagedevice) never pushes any dictionaries on the dict stack - if that changes, we should check that stack, too. --- psi/zdevice2.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/psi/zdevice2.c b/psi/zdevice2.c index 5447c8c..159a0c0 100644 --- a/psi/zdevice2.c +++ b/psi/zdevice2.c @@ -278,8 +278,8 @@ restore_page_device(i_ctx_t *i_ctx_p, const gs_gstate * pgs_old, const gs_gstate } if (LockSafetyParams && !samepagedevice) { - os_ptr op = osp; - const int max_ops = 512; + const int required_ops = 512; + const int required_es = 32; /* The %grestorepagedevice must complete: the biggest danger is operand stack overflow. As we use get/putdeviceparams @@ -289,9 +289,16 @@ restore_page_device(i_ctx_t *i_ctx_p, const gs_gstate * pgs_old, const gs_gstate 424 entries on the op stack. Allowing for working stack space, and safety margin..... */ - if (max_ops > op - osbot) { - if (max_ops >= ref_stack_count(&o_stack)) - return_error(gs_error_stackoverflow); + if (required_ops + ref_stack_count(&o_stack) >= ref_stack_max_count(&o_stack)) { + gs_currentdevice(pgs_old)->LockSafetyParams = LockSafetyParams; + return_error(gs_error_stackoverflow); + } + /* We also want enough exec stack space - 32 is an overestimate of + what we need to complete the Postscript call out. + */ + if (required_es + ref_stack_count(&e_stack) >= ref_stack_max_count(&e_stack)) { + gs_currentdevice(pgs_old)->LockSafetyParams = LockSafetyParams; + return_error(gs_error_execstackoverflow); } } /* -- 2.14.4