James Harvey points out that entering an invalid game ID can affect

the current midend state even if you don't subsequently enter a
valid one. Reorganise midend_game_id_int() so that (just like
midend_deserialise()) it does all its error checking before altering
anything in the midend's persistent data, so that it either succeeds
completely or fails before doing anything at all.

[originally from svn r6045]

1 files changed, 54 insertions, 16 deletions

diff --git a/midend.c b/midend.c
index a764c77..7bc2d4f 100644
--- a/midend.c
+++ b/midend.c
@@ -855,6 +855,8 @@ config_item *midend_get_config(midend_data *me, int which, char **wintitle)
 static char *midend_game_id_int(midend_data *me, char *id, int defmode)
 {
     char *error, *par, *desc, *seed;
+    game_params *newcurparams, *newparams, *oldparams1, *oldparams2;
+    int free_params;
 
     seed = strchr(id, '#');
     desc = strchr(id, ':');
@@ -894,18 +896,24 @@ static char *midend_game_id_int(midend_data *me, char *id, int defmode)
         }
     }
 
+    /*
+     * We must be reasonably careful here not to modify anything in
+     * `me' until we have finished validating things. This function
+     * must either return an error and do nothing to the midend, or
+     * return success and do everything; nothing in between is
+     * acceptable.
+     */
+    newcurparams = newparams = oldparams1 = oldparams2 = NULL;
+
     if (par) {
-        game_params *tmpparams;
-        tmpparams = me->ourgame->dup_params(me->params);
-        me->ourgame->decode_params(tmpparams, par);
-        error = me->ourgame->validate_params(tmpparams);
+        newcurparams = me->ourgame->dup_params(me->params);
+        me->ourgame->decode_params(newcurparams, par);
+        error = me->ourgame->validate_params(newcurparams);
         if (error) {
-            me->ourgame->free_params(tmpparams);
+            me->ourgame->free_params(newcurparams);
             return error;
         }
-        if (me->curparams)
-            me->ourgame->free_params(me->curparams);
-        me->curparams = tmpparams;
+        oldparams1 = me->curparams;
 
         /*
          * Now filter only the persistent parts of this state into
@@ -913,16 +921,50 @@ static char *midend_game_id_int(midend_data *me, char *id, int defmode)
          * received a params string in which case the whole lot is
          * persistent.
          */
+        oldparams2 = me->params;
         if (seed || desc) {
-            char *tmpstr = me->ourgame->encode_params(tmpparams, FALSE);
-            me->ourgame->decode_params(me->params, tmpstr);
+            char *tmpstr;
+
+            newparams = me->ourgame->dup_params(me->params);
+
+            tmpstr = me->ourgame->encode_params(newcurparams, FALSE);
+            me->ourgame->decode_params(newparams, tmpstr);
+
             sfree(tmpstr);
         } else {
-            me->ourgame->free_params(me->params);
-            me->params = me->ourgame->dup_params(tmpparams);
+            newparams = me->ourgame->dup_params(newcurparams);
         }
+        free_params = TRUE;
+    } else {
+        newcurparams = me->curparams;
+        newparams = me->params;
+        free_params = FALSE;
     }
 
+    if (desc) {
+        error = me->ourgame->validate_desc(newparams, desc);
+        if (error) {
+            if (free_params) {
+                if (newcurparams)
+                    me->ourgame->free_params(newcurparams);
+                if (newparams)
+                    me->ourgame->free_params(newparams);
+            }
+            return error;
+        }
+    }
+
+    /*
+     * Now we've got past all possible error points. Update the
+     * midend itself.
+     */
+    me->params = newparams;
+    me->curparams = newcurparams;
+    if (oldparams1)
+        me->ourgame->free_params(oldparams1);
+    if (oldparams2)
+        me->ourgame->free_params(oldparams2);
+
     sfree(me->desc);
     sfree(me->privdesc);
     me->desc = me->privdesc = NULL;
@@ -930,10 +972,6 @@ static char *midend_game_id_int(midend_data *me, char *id, int defmode)
     me->seedstr = NULL;
 
     if (desc) {
-        error = me->ourgame->validate_desc(me->params, desc);
-        if (error)
-            return error;
-
         me->desc = dupstr(desc);
         me->genmode = GOT_DESC;
         sfree(me->aux_info);