From: Chris Liddell Date: Fri, 24 Aug 2018 08:26:04 +0000 (+0100) Subject: Improve restore robustness X-Git-Tag: ghostpdl-9.24rc1~10 X-Git-Url: http://git.ghostscript.com/?p=ghostpdl.git;a=commitdiff_plain;h=5516c614 Improve restore robustness Prompted by looking at Bug 699654: There are two variants of the restore operator in Ghostscript: one is Level 1 (restoring VM), the other is Level 2+ (adding page device restoring to the Level operator). This was implemented by the Level 2+ version restoring the device in the graphics state, then calling the Level 1 implementation to handle actually restoring the VM state. The problem was that the operand checking, and sanity of the save object was only done by the Level 1 variant, thus meaning an invalid save object could leave a (Level 2+) restore partially complete - with the page device part restored, but not VM, and the page device not configured. To solve that, this commit splits the operand and sanity checking, and the core of the restore operation into separate functions, so the relevant operators can validate the operand *before* taking any further action. That reduces the chances of an invalid restore leaving the interpreter in an unknown state. If an error occurs during the actual VM restore it is essentially fatal, and the interpreter cannot continue, but as an extra surety for security, in the event of such an error, we'll explicitly preserve the LockSafetyParams of the device, rather than rely on the post-restore device configuration (which won't happen in the event of an error). --- diff --git a/psi/int.mak b/psi/int.mak index 1968820..16db0cf 100644 --- a/psi/int.mak +++ b/psi/int.mak @@ -1086,8 +1086,8 @@ $(PSD)pagedev.dev : $(ECHOGS_XE) $(pagedev_)\ $(PSOBJ)zdevice2.$(OBJ) : $(PSSRC)zdevice2.c $(OP) $(math__h) $(memory__h)\ $(dstack_h) $(estack_h)\ - $(idict_h) $(idparam_h) $(igstate_h) $(iname_h) $(iutil_h) $(store_h)\ - $(gxdevice_h) $(gsstate_h) $(INT_MAK) $(MAKEDIRS) + $(idict_h) $(idparam_h) $(igstate_h) $(iname_h) $(isave) $(iutil_h) \ + $(store_h) $(gxdevice_h) $(gsstate_h) $(INT_MAK) $(MAKEDIRS) $(PSCC) $(PSO_)zdevice2.$(OBJ) $(C_) $(PSSRC)zdevice2.c $(PSOBJ)zmedia2.$(OBJ) : $(PSSRC)zmedia2.c $(OP) $(math__h) $(memory__h)\ diff --git a/psi/isave.h b/psi/isave.h index 3021639..7eaaced 100644 --- a/psi/isave.h +++ b/psi/isave.h @@ -128,4 +128,10 @@ int font_restore(const alloc_save_t * save); express purpose of getting the library context. */ gs_memory_t *gs_save_any_memory(const alloc_save_t *save); +int +restore_check_save(i_ctx_t *i_ctx_p, alloc_save_t **asave); + +int +dorestore(i_ctx_t *i_ctx_p, alloc_save_t *asave); + #endif /* isave_INCLUDED */ diff --git a/psi/zdevice2.c b/psi/zdevice2.c index 9fbb4e3..0c7080d 100644 --- a/psi/zdevice2.c +++ b/psi/zdevice2.c @@ -26,6 +26,7 @@ #include "igstate.h" #include "iname.h" #include "iutil.h" +#include "isave.h" #include "store.h" #include "gxdevice.h" #include "gsstate.h" @@ -307,13 +308,24 @@ z2grestoreall(i_ctx_t *i_ctx_p) } return 0; } - +/* This is the Level 2+ variant of restore - which adds restoring + of the page device to the Level 1 variant in zvmem.c. + Previous this restored the device state before calling zrestore.c + which validated operands etc, meaning a restore could error out + partially complete. + The operand checking, and actual VM restore are now in two functions + so they can called separately thus, here, we can do as much + checking as possible, before embarking on actual changes + */ /* restore - */ static int z2restore(i_ctx_t *i_ctx_p) { - os_ptr op = osp; - check_type(*op, t_save); + alloc_save_t *asave; + bool saveLockSafety = gs_currentdevice_inline(igs)->LockSafetyParams; + int code = restore_check_save(i_ctx_p, &asave); + + if (code < 0) return code; while (gs_gstate_saved(gs_gstate_saved(igs))) { if (restore_page_device(igs, gs_gstate_saved(igs))) @@ -322,7 +334,20 @@ z2restore(i_ctx_t *i_ctx_p) } if (restore_page_device(igs, gs_gstate_saved(igs))) return push_callout(i_ctx_p, "%restorepagedevice"); - return zrestore(i_ctx_p); + + code = dorestore(i_ctx_p, asave); + + if (code < 0) { + /* An error here is basically fatal, but.... + restore_page_device() has to set LockSafetyParams false so it can + configure the restored device correctly - in normal operation, that + gets reset by that configuration. If we hit an error, though, that + may not happen - at least ensure we keep the setting through the + error. + */ + gs_currentdevice_inline(igs)->LockSafetyParams = saveLockSafety; + } + return code; } /* setgstate - */ diff --git a/psi/zvmem.c b/psi/zvmem.c index 44cd7a8..87a0a4f 100644 --- a/psi/zvmem.c +++ b/psi/zvmem.c @@ -99,19 +99,18 @@ zsave(i_ctx_t *i_ctx_p) static int restore_check_operand(os_ptr, alloc_save_t **, gs_dual_memory_t *); static int restore_check_stack(const i_ctx_t *i_ctx_p, const ref_stack_t *, const alloc_save_t *, bool); static void restore_fix_stack(i_ctx_t *i_ctx_p, ref_stack_t *, const alloc_save_t *, bool); + +/* Do as many up front checks of the save object as we reasonably can */ int -zrestore(i_ctx_t *i_ctx_p) +restore_check_save(i_ctx_t *i_ctx_p, alloc_save_t **asave) { os_ptr op = osp; - alloc_save_t *asave; - bool last; - vm_save_t *vmsave; - int code = restore_check_operand(op, &asave, idmemory); + int code = restore_check_operand(op, asave, idmemory); if (code < 0) return code; if_debug2m('u', imemory, "[u]vmrestore 0x%lx, id = %lu\n", - (ulong) alloc_save_client_data(asave), + (ulong) alloc_save_client_data(*asave), (ulong) op->value.saveid); if (I_VALIDATE_BEFORE_RESTORE) ivalidate_clean_spaces(i_ctx_p); @@ -120,14 +119,37 @@ zrestore(i_ctx_t *i_ctx_p) { int code; - if ((code = restore_check_stack(i_ctx_p, &o_stack, asave, false)) < 0 || - (code = restore_check_stack(i_ctx_p, &e_stack, asave, true)) < 0 || - (code = restore_check_stack(i_ctx_p, &d_stack, asave, false)) < 0 + if ((code = restore_check_stack(i_ctx_p, &o_stack, *asave, false)) < 0 || + (code = restore_check_stack(i_ctx_p, &e_stack, *asave, true)) < 0 || + (code = restore_check_stack(i_ctx_p, &d_stack, *asave, false)) < 0 ) { osp++; return code; } } + osp++; + return 0; +} + +/* the semantics of restore differ slightly between Level 1 and + Level 2 and later - the latter includes restoring the device + state (whilst Level 1 didn't have "page devices" as such). + Hence we have two restore operators - one here (Level 1) + and one in zdevice2.c (Level 2+). For that reason, the + operand checking and guts of the restore operation are + separated so both implementations can use them to best + effect. + */ +int +dorestore(i_ctx_t *i_ctx_p, alloc_save_t *asave) +{ + os_ptr op = osp; + bool last; + vm_save_t *vmsave; + int code; + + osp--; + /* Reset l_new in all stack entries if the new save level is zero. */ /* Also do some special fixing on the e-stack. */ restore_fix_stack(i_ctx_p, &o_stack, asave, false); @@ -170,9 +192,24 @@ zrestore(i_ctx_t *i_ctx_p) /* cause an 'invalidaccess' in setuserparams. Temporarily set */ /* LockFilePermissions false until the gs_lev2.ps can do a */ /* setuserparams from the restored userparam dictionary. */ + /* NOTE: This is safe to do here, since the restore has */ + /* successfully completed - this should never come before any */ + /* operation that can trigger an error */ i_ctx_p->LockFilePermissions = false; return 0; } + +int +zrestore(i_ctx_t *i_ctx_p) +{ + alloc_save_t *asave; + int code = restore_check_save(i_ctx_p, &asave); + if (code < 0) + return code; + + return dorestore(i_ctx_p, asave); +} + /* Check the operand of a restore. */ static int restore_check_operand(os_ptr op, alloc_save_t ** pasave, @@ -193,6 +230,7 @@ restore_check_operand(os_ptr op, alloc_save_t ** pasave, *pasave = asave; return 0; } + /* Check a stack to make sure all its elements are older than a save. */ static int restore_check_stack(const i_ctx_t *i_ctx_p, const ref_stack_t * pstack,