core: Jim_SetVariable() now frees value on errormaster-next

If the value has a zero reference count, free it in the error path.
Otherwise callers can't do: Jim_SetVariable(..., Jim_New...())
in case the object is leaked in the error path.

Fix various callers that were previously freeing the object in the
error path, and add a test to loop.test to show that this was not.

Signed-off-by: Steve Bennett <steveb@workware.net.au>

7 files changed, 31 insertions, 44 deletions

diff --git a/jim-aio.c b/jim-aio.c
index 67483a4..473b4e7 100644
--- a/jim-aio.c
+++ b/jim-aio.c
@@ -637,9 +637,7 @@ static int JimSetVariableSocketAddress(Jim_Interp *interp, Jim_Obj *varObjPtr, c
 {
     int ret;
     Jim_Obj *objPtr = JimFormatSocketAddress(interp, sa, salen);
-    Jim_IncrRefCount(objPtr);
     ret = Jim_SetVariable(interp, varObjPtr, objPtr);
-    Jim_DecrRefCount(interp, objPtr);
     return ret;
 }
 
@@ -1181,7 +1179,6 @@ static int aio_cmd_gets(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
 
     if (argc) {
         if (Jim_SetVariable(interp, argv[0], objPtr) != JIM_OK) {
-            Jim_FreeNewObj(interp, objPtr);
             return JIM_ERR;
         }
 
diff --git a/jim-history.c b/jim-history.c
index c43bdca..39ddf5d 100644
--- a/jim-history.c
+++ b/jim-history.c
@@ -24,7 +24,6 @@ static int history_cmd_getline(Jim_Interp *interp, int argc, Jim_Obj *const *arg
     /* Returns the length of the string if varName was specified */
     if (argc == 2) {
         if (Jim_SetVariable(interp, argv[1], objPtr) != JIM_OK) {
-            Jim_FreeNewObj(interp, objPtr);
             return JIM_ERR;
         }
         Jim_SetResultInt(interp, Jim_Length(objPtr));
diff --git a/jim-pack.c b/jim-pack.c
index e3395b6..b567cec 100644
--- a/jim-pack.c
+++ b/jim-pack.c
@@ -371,7 +371,6 @@ static int Jim_PackCmd(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
     double fvalue;
     Jim_Obj *stringObjPtr;
     int len;
-    int freeobj = 0;
 
     if (Jim_GetEnum(interp, argv[3], options, &option, NULL, JIM_ERRMSG) != JIM_OK) {
         return JIM_ERR;
@@ -406,10 +405,8 @@ static int Jim_PackCmd(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
     if (!stringObjPtr) {
         /* Create the string if it doesn't exist */
         stringObjPtr = Jim_NewEmptyStringObj(interp);
-        freeobj = 1;
     }
     else if (Jim_IsShared(stringObjPtr)) {
-        freeobj = 1;
         stringObjPtr = Jim_DuplicateObj(interp, stringObjPtr);
     }
 
@@ -455,10 +452,7 @@ static int Jim_PackCmd(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
     }
 
     if (Jim_SetVariable(interp, argv[1], stringObjPtr) != JIM_OK) {
-        if (freeobj) {
-            Jim_FreeNewObj(interp, stringObjPtr);
-            return JIM_ERR;
-        }
+        return JIM_ERR;
     }
     return JIM_OK;
 }
diff --git a/jim-regexp.c b/jim-regexp.c
index 22e69eb..d19867e 100644
--- a/jim-regexp.c
+++ b/jim-regexp.c
@@ -308,7 +308,6 @@ int Jim_RegexpCmd(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
             result = Jim_SetVariable(interp, argv[i + 2 + j], resultObj);
 
             if (result != JIM_OK) {
-                Jim_FreeObj(interp, resultObj);
                 break;
             }
         }
@@ -618,9 +617,6 @@ cmd_error:
             if (result == JIM_OK) {
                 Jim_SetResultInt(interp, num_matches);
             }
-            else {
-                Jim_FreeObj(interp, resultObj);
-            }
         }
         else {
             Jim_SetResult(interp, resultObj);
diff --git a/jim.c b/jim.c
index 1454bc4..fd38080 100644
--- a/jim.c
+++ b/jim.c
@@ -4689,15 +4689,18 @@ static Jim_VarVal *JimCreateVariable(Jim_Interp *interp, Jim_Obj *nameObjPtr, Ji
  */
 int Jim_SetVariable(Jim_Interp *interp, Jim_Obj *nameObjPtr, Jim_Obj *valObjPtr)
 {
-    int err;
+    int ret = JIM_OK;
     Jim_VarVal *vv;
 
     switch (SetVariableFromAny(interp, nameObjPtr)) {
         case JIM_DICT_SUGAR:
-            return JimDictSugarSet(interp, nameObjPtr, valObjPtr);
+            ret = JimDictSugarSet(interp, nameObjPtr, valObjPtr);
+            break;
 
         case JIM_ERR:
-            JimCreateVariable(interp, nameObjPtr, valObjPtr);
+            if (JimCreateVariable(interp, nameObjPtr, valObjPtr) == NULL) {
+                ret = JIM_ERR;
+            }
             break;
 
         case JIM_OK:
@@ -4712,13 +4715,16 @@ int Jim_SetVariable(Jim_Interp *interp, Jim_Obj *nameObjPtr, Jim_Obj *valObjPtr)
 
                 savedCallFrame = interp->framePtr;
                 interp->framePtr = vv->linkFramePtr;
-                err = Jim_SetVariable(interp, vv->objPtr, valObjPtr);
+                ret = Jim_SetVariable(interp, vv->objPtr, valObjPtr);
                 interp->framePtr = savedCallFrame;
-                if (err != JIM_OK)
-                    return err;
             }
+            break;
     }
-    return JIM_OK;
+    if (ret != JIM_OK && valObjPtr->refCount == 0) {
+        /* Since we couldn't take ownership of the object, need to free it here */
+        Jim_FreeNewObj(interp, valObjPtr);
+    }
+    return ret;
 }
 
 int Jim_SetVariableStr(Jim_Interp *interp, const char *name, Jim_Obj *objPtr)
@@ -7403,8 +7409,9 @@ int Jim_ListSetIndex(Jim_Interp *interp, Jim_Obj *varNamePtr,
         goto err;
     Jim_InvalidateStringRep(objPtr);
     Jim_InvalidateStringRep(varObjPtr);
-    if (Jim_SetVariable(interp, varNamePtr, varObjPtr) != JIM_OK)
-        goto err;
+    if (Jim_SetVariable(interp, varNamePtr, varObjPtr) != JIM_OK) {
+        return JIM_ERR;
+    }
     Jim_SetResult(interp, varObjPtr);
     return JIM_OK;
   err:
@@ -8040,7 +8047,6 @@ int Jim_SetDictKeysVector(Jim_Interp *interp, Jim_Obj *varNamePtr,
         }
         varObjPtr = objPtr = Jim_NewDictObj(interp, NULL, 0);
         if (Jim_SetVariable(interp, varNamePtr, objPtr) != JIM_OK) {
-            Jim_FreeNewObj(interp, varObjPtr);
             return JIM_ERR;
         }
     }
@@ -8095,7 +8101,7 @@ int Jim_SetDictKeysVector(Jim_Interp *interp, Jim_Obj *varNamePtr,
     Jim_InvalidateStringRep(objPtr);
     Jim_InvalidateStringRep(varObjPtr);
     if (Jim_SetVariable(interp, varNamePtr, varObjPtr) != JIM_OK) {
-        goto err;
+        return JIM_ERR;
     }
 
     if (!(flags & JIM_NORESULT)) {
@@ -10832,7 +10838,6 @@ static int Jim_IncrCoreCommand(Jim_Interp *interp, int argc, Jim_Obj *const *arg
     if (!intObjPtr || Jim_IsShared(intObjPtr)) {
         intObjPtr = Jim_NewIntObj(interp, wideValue + increment);
         if (Jim_SetVariable(interp, argv[1], intObjPtr) != JIM_OK) {
-            Jim_FreeNewObj(interp, intObjPtr);
             return JIM_ERR;
         }
     }
@@ -12929,9 +12934,6 @@ static int Jim_LoopCoreCommand(Jim_Interp *interp, int argc, Jim_Obj *const *arg
             else {
                 objPtr = Jim_NewIntObj(interp, i);
                 retval = Jim_SetVariable(interp, argv[1], objPtr);
-                if (retval != JIM_OK) {
-                    Jim_FreeNewObj(interp, objPtr);
-                }
             }
         }
     }
@@ -13044,10 +13046,10 @@ static int JimForeachMapHelper(Jim_Interp *interp, int argc, Jim_Obj *const *arg
                     /* Ran out, so store the empty string */
                     valObj = interp->emptyObj;
                 }
-                /* Avoid shimmering */
-                Jim_IncrRefCount(valObj);
+                // XXX
+                //Jim_IncrRefCount(valObj);
                 result = Jim_SetVariable(interp, varName, valObj);
-                Jim_DecrRefCount(interp, valObj);
+                //Jim_DecrRefCount(interp, valObj);
                 if (result != JIM_OK) {
                     goto err;
                 }
@@ -13572,24 +13574,19 @@ static int Jim_LsearchCoreCommand(Jim_Interp *interp, int argc, Jim_Obj *const *
 static int Jim_LappendCoreCommand(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
 {
     Jim_Obj *listObjPtr;
-    int new_obj = 0;
     int i;
 
     listObjPtr = Jim_GetVariable(interp, argv[1], JIM_UNSHARED);
     if (!listObjPtr) {
         /* Create the list if it does not exist */
         listObjPtr = Jim_NewListObj(interp, NULL, 0);
-        new_obj = 1;
     }
     else if (Jim_IsShared(listObjPtr)) {
         listObjPtr = Jim_DuplicateObj(interp, listObjPtr);
-        new_obj = 1;
     }
     for (i = 2; i < argc; i++)
         Jim_ListAppendElement(interp, listObjPtr, argv[i]);
     if (Jim_SetVariable(interp, argv[1], listObjPtr) != JIM_OK) {
-        if (new_obj)
-            Jim_FreeNewObj(interp, listObjPtr);
         return JIM_ERR;
     }
     Jim_SetResult(interp, listObjPtr);
@@ -13832,24 +13829,18 @@ static int Jim_AppendCoreCommand(Jim_Interp *interp, int argc, Jim_Obj *const *a
             return JIM_ERR;
     }
     else {
-        int new_obj = 0;
         stringObjPtr = Jim_GetVariable(interp, argv[1], JIM_UNSHARED);
         if (!stringObjPtr) {
             /* Create the string if it doesn't exist */
             stringObjPtr = Jim_NewEmptyStringObj(interp);
-            new_obj = 1;
         }
         else if (Jim_IsShared(stringObjPtr)) {
-            new_obj = 1;
             stringObjPtr = Jim_DuplicateObj(interp, stringObjPtr);
         }
         for (i = 2; i < argc; i++) {
             Jim_AppendObj(interp, stringObjPtr, argv[i]);
         }
         if (Jim_SetVariable(interp, argv[1], stringObjPtr) != JIM_OK) {
-            if (new_obj) {
-                Jim_FreeNewObj(interp, stringObjPtr);
-            }
             return JIM_ERR;
         }
     }
diff --git a/jim.h b/jim.h
index 430f396..16130dd 100644
--- a/jim.h
+++ b/jim.h
@@ -838,6 +838,7 @@ JIM_EXPORT int Jim_RenameCommand (Jim_Interp *interp,
         Jim_Obj *oldNameObj, Jim_Obj *newNameObj);
 JIM_EXPORT Jim_Cmd * Jim_GetCommand (Jim_Interp *interp,
         Jim_Obj *objPtr, int flags);
+/* Note that if Jim_SetVariable() fails, and valObjPtr has a zero reference count, it will be freed */
 JIM_EXPORT int Jim_SetVariable (Jim_Interp *interp,
         Jim_Obj *nameObjPtr, Jim_Obj *valObjPtr);
 JIM_EXPORT int Jim_SetVariableStr (Jim_Interp *interp,
diff --git a/tests/loop.test b/tests/loop.test
index c6144e0..43fe562 100644
--- a/tests/loop.test
+++ b/tests/loop.test
@@ -152,6 +152,15 @@ test loop-2.8 {modify loop var} {
     set a
 } {1 2 3 4 5}
 
+# Previously this would leak memory (configure --maintainer)
+test loop-2.9 {fail to set loop var} -body {
+    set i 1
+    loop i(x) 1 6 {
+        incr y
+    }
+    set y
+} -returnCodes error -result {can't set "i(x)": variable isn't array}
+
 testreport
 break