From 26d93b9a3f2a47ece821defe12e9059b4a7a3bf2 Mon Sep 17 00:00:00 2001

From: Chris Liddell <chris.liddell@artifex.com>

Date: Fri, 24 Aug 2018 09:26:04 +0100

Subject: [PATCH] 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).

---

 psi/int.mak    |  4 ++--

 psi/isave.h    |  6 ++++++

 psi/zdevice2.c | 32 ++++++++++++++++++++++++++++++--

 psi/zvmem.c    | 56 +++++++++++++++++++++++++++++++++++++++++++++++---------

 4 files changed, 85 insertions(+), 13 deletions(-)

diff --git a/psi/int.mak b/psi/int.mak

index fbf191d..4d0f296 100644

--- a/psi/int.mak

+++ b/psi/int.mak

@@ -1047,8 +1047,8 @@ $(PSD)pagedev.dev : $(INT_MAK) $(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)

+ $(idict_h) $(idparam_h) $(igstate_h) $(iname_h) $(isave) $(iutil_h) \

+ $(store_h) $(gxdevice_h) $(gsstate_h)

 	$(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 f8a6f32..0914de1 100644

--- a/psi/isave.h

+++ b/psi/isave.h

@@ -122,4 +122,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 f391da1..2ed2a3b 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"

@@ -306,11 +307,25 @@ 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

+ */

 /* <save> restore - */

 static int

 z2restore(i_ctx_t *i_ctx_p)

 {

+    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_state_saved(gs_state_saved(igs))) {

         if (restore_page_device(igs, gs_state_saved(igs)))

             return push_callout(i_ctx_p, "%restore1pagedevice");

@@ -318,7 +333,20 @@ z2restore(i_ctx_t *i_ctx_p)

     }

     if (restore_page_device(igs, gs_state_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;

 }

 /* <gstate> setgstate - */

diff --git a/psi/zvmem.c b/psi/zvmem.c

index 4ad8f9a..d8de39d 100644

--- a/psi/zvmem.c

+++ b/psi/zvmem.c

@@ -104,19 +104,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);

@@ -125,14 +124,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);

@@ -175,9 +197,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,

@@ -198,6 +235,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,

-- 

2.14.4