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).

4 files changed, 84 insertions, 15 deletions

diff --git a/psi/int.mak b/psi/int.mak
index 19688202a..16db0cff0 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 30216398f..7eaaced46 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 9fbb4e339..0c7080d57 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
+ */
 /* <save> 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;
 }
 
 /* <gstate> setgstate - */
diff --git a/psi/zvmem.c b/psi/zvmem.c
index 44cd7a8e0..87a0a4ff1 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,