Skip to content

Commit f8bf4be

Browse files
mstevebmsteveb
committed
core: Jim_SetVariable() now frees value on error
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 <[email protected]>
1 parent 8d8df96 commit f8bf4be

File tree

7 files changed

+31
-44
lines changed

7 files changed

+31
-44
lines changed

‎jim-aio.c‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -637,9 +637,7 @@ static int JimSetVariableSocketAddress(Jim_Interp *interp, Jim_Obj *varObjPtr, c
637637
{
638638
int ret;
639639
Jim_Obj *objPtr = JimFormatSocketAddress(interp, sa, salen);
640-
Jim_IncrRefCount(objPtr);
641640
ret = Jim_SetVariable(interp, varObjPtr, objPtr);
642-
Jim_DecrRefCount(interp, objPtr);
643641
return ret;
644642
}
645643

@@ -1181,7 +1179,6 @@ static int aio_cmd_gets(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
11811179

11821180
if (argc) {
11831181
if (Jim_SetVariable(interp, argv[0], objPtr) != JIM_OK) {
1184-
Jim_FreeNewObj(interp, objPtr);
11851182
return JIM_ERR;
11861183
}
11871184

‎jim-history.c‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ static int history_cmd_getline(Jim_Interp *interp, int argc, Jim_Obj *const *arg
2424
/* Returns the length of the string if varName was specified */
2525
if (argc == 2) {
2626
if (Jim_SetVariable(interp, argv[1], objPtr) != JIM_OK) {
27-
Jim_FreeNewObj(interp, objPtr);
2827
return JIM_ERR;
2928
}
3029
Jim_SetResultInt(interp, Jim_Length(objPtr));

‎jim-pack.c‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -371,7 +371,6 @@ static int Jim_PackCmd(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
371371
double fvalue;
372372
Jim_Obj *stringObjPtr;
373373
int len;
374-
int freeobj = 0;
375374

376375
if (Jim_GetEnum(interp, argv[3], options, &option, NULL, JIM_ERRMSG) != JIM_OK) {
377376
return JIM_ERR;
@@ -406,10 +405,8 @@ static int Jim_PackCmd(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
406405
if (!stringObjPtr) {
407406
/* Create the string if it doesn't exist */
408407
stringObjPtr = Jim_NewEmptyStringObj(interp);
409-
freeobj = 1;
410408
}
411409
else if (Jim_IsShared(stringObjPtr)) {
412-
freeobj = 1;
413410
stringObjPtr = Jim_DuplicateObj(interp, stringObjPtr);
414411
}
415412

@@ -455,10 +452,7 @@ static int Jim_PackCmd(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
455452
}
456453

457454
if (Jim_SetVariable(interp, argv[1], stringObjPtr) != JIM_OK) {
458-
if (freeobj) {
459-
Jim_FreeNewObj(interp, stringObjPtr);
460-
return JIM_ERR;
461-
}
455+
return JIM_ERR;
462456
}
463457
return JIM_OK;
464458
}

‎jim-regexp.c‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,6 @@ int Jim_RegexpCmd(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
308308
result = Jim_SetVariable(interp, argv[i + 2 + j], resultObj);
309309

310310
if (result != JIM_OK) {
311-
Jim_FreeObj(interp, resultObj);
312311
break;
313312
}
314313
}
@@ -618,9 +617,6 @@ int Jim_RegsubCmd(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
618617
if (result == JIM_OK) {
619618
Jim_SetResultInt(interp, num_matches);
620619
}
621-
else {
622-
Jim_FreeObj(interp, resultObj);
623-
}
624620
}
625621
else {
626622
Jim_SetResult(interp, resultObj);

‎jim.c‎

Lines changed: 20 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -4689,15 +4689,18 @@ static Jim_VarVal *JimCreateVariable(Jim_Interp *interp, Jim_Obj *nameObjPtr, Ji
46894689
*/
46904690
int Jim_SetVariable(Jim_Interp *interp, Jim_Obj *nameObjPtr, Jim_Obj *valObjPtr)
46914691
{
4692-
int err;
4692+
int ret = JIM_OK;
46934693
Jim_VarVal *vv;
46944694

46954695
switch (SetVariableFromAny(interp, nameObjPtr)) {
46964696
case JIM_DICT_SUGAR:
4697-
return JimDictSugarSet(interp, nameObjPtr, valObjPtr);
4697+
ret = JimDictSugarSet(interp, nameObjPtr, valObjPtr);
4698+
break;
46984699

46994700
case JIM_ERR:
4700-
JimCreateVariable(interp, nameObjPtr, valObjPtr);
4701+
if (JimCreateVariable(interp, nameObjPtr, valObjPtr) == NULL) {
4702+
ret = JIM_ERR;
4703+
}
47014704
break;
47024705

47034706
case JIM_OK:
@@ -4712,13 +4715,16 @@ int Jim_SetVariable(Jim_Interp *interp, Jim_Obj *nameObjPtr, Jim_Obj *valObjPtr)
47124715

47134716
savedCallFrame = interp->framePtr;
47144717
interp->framePtr = vv->linkFramePtr;
4715-
err = Jim_SetVariable(interp, vv->objPtr, valObjPtr);
4718+
ret = Jim_SetVariable(interp, vv->objPtr, valObjPtr);
47164719
interp->framePtr = savedCallFrame;
4717-
if (err != JIM_OK)
4718-
return err;
47194720
}
4721+
break;
47204722
}
4721-
return JIM_OK;
4723+
if (ret != JIM_OK && valObjPtr->refCount == 0) {
4724+
/* Since we couldn't take ownership of the object, need to free it here */
4725+
Jim_FreeNewObj(interp, valObjPtr);
4726+
}
4727+
return ret;
47224728
}
47234729

47244730
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,
74037409
goto err;
74047410
Jim_InvalidateStringRep(objPtr);
74057411
Jim_InvalidateStringRep(varObjPtr);
7406-
if (Jim_SetVariable(interp, varNamePtr, varObjPtr) != JIM_OK)
7407-
goto err;
7412+
if (Jim_SetVariable(interp, varNamePtr, varObjPtr) != JIM_OK) {
7413+
return JIM_ERR;
7414+
}
74087415
Jim_SetResult(interp, varObjPtr);
74097416
return JIM_OK;
74107417
err:
@@ -8040,7 +8047,6 @@ int Jim_SetDictKeysVector(Jim_Interp *interp, Jim_Obj *varNamePtr,
80408047
}
80418048
varObjPtr = objPtr = Jim_NewDictObj(interp, NULL, 0);
80428049
if (Jim_SetVariable(interp, varNamePtr, objPtr) != JIM_OK) {
8043-
Jim_FreeNewObj(interp, varObjPtr);
80448050
return JIM_ERR;
80458051
}
80468052
}
@@ -8095,7 +8101,7 @@ int Jim_SetDictKeysVector(Jim_Interp *interp, Jim_Obj *varNamePtr,
80958101
Jim_InvalidateStringRep(objPtr);
80968102
Jim_InvalidateStringRep(varObjPtr);
80978103
if (Jim_SetVariable(interp, varNamePtr, varObjPtr) != JIM_OK) {
8098-
goto err;
8104+
return JIM_ERR;
80998105
}
81008106

81018107
if (!(flags & JIM_NORESULT)) {
@@ -10832,7 +10838,6 @@ static int Jim_IncrCoreCommand(Jim_Interp *interp, int argc, Jim_Obj *const *arg
1083210838
if (!intObjPtr || Jim_IsShared(intObjPtr)) {
1083310839
intObjPtr = Jim_NewIntObj(interp, wideValue + increment);
1083410840
if (Jim_SetVariable(interp, argv[1], intObjPtr) != JIM_OK) {
10835-
Jim_FreeNewObj(interp, intObjPtr);
1083610841
return JIM_ERR;
1083710842
}
1083810843
}
@@ -12929,9 +12934,6 @@ static int Jim_LoopCoreCommand(Jim_Interp *interp, int argc, Jim_Obj *const *arg
1292912934
else {
1293012935
objPtr = Jim_NewIntObj(interp, i);
1293112936
retval = Jim_SetVariable(interp, argv[1], objPtr);
12932-
if (retval != JIM_OK) {
12933-
Jim_FreeNewObj(interp, objPtr);
12934-
}
1293512937
}
1293612938
}
1293712939
}
@@ -13044,10 +13046,10 @@ static int JimForeachMapHelper(Jim_Interp *interp, int argc, Jim_Obj *const *arg
1304413046
/* Ran out, so store the empty string */
1304513047
valObj = interp->emptyObj;
1304613048
}
13047-
/* Avoid shimmering */
13048-
Jim_IncrRefCount(valObj);
13049+
// XXX
13050+
//Jim_IncrRefCount(valObj);
1304913051
result = Jim_SetVariable(interp, varName, valObj);
13050-
Jim_DecrRefCount(interp, valObj);
13052+
//Jim_DecrRefCount(interp, valObj);
1305113053
if (result != JIM_OK) {
1305213054
goto err;
1305313055
}
@@ -13572,24 +13574,19 @@ static int Jim_LsearchCoreCommand(Jim_Interp *interp, int argc, Jim_Obj *const *
1357213574
static int Jim_LappendCoreCommand(Jim_Interp *interp, int argc, Jim_Obj *const *argv)
1357313575
{
1357413576
Jim_Obj *listObjPtr;
13575-
int new_obj = 0;
1357613577
int i;
1357713578

1357813579
listObjPtr = Jim_GetVariable(interp, argv[1], JIM_UNSHARED);
1357913580
if (!listObjPtr) {
1358013581
/* Create the list if it does not exist */
1358113582
listObjPtr = Jim_NewListObj(interp, NULL, 0);
13582-
new_obj = 1;
1358313583
}
1358413584
else if (Jim_IsShared(listObjPtr)) {
1358513585
listObjPtr = Jim_DuplicateObj(interp, listObjPtr);
13586-
new_obj = 1;
1358713586
}
1358813587
for (i = 2; i < argc; i++)
1358913588
Jim_ListAppendElement(interp, listObjPtr, argv[i]);
1359013589
if (Jim_SetVariable(interp, argv[1], listObjPtr) != JIM_OK) {
13591-
if (new_obj)
13592-
Jim_FreeNewObj(interp, listObjPtr);
1359313590
return JIM_ERR;
1359413591
}
1359513592
Jim_SetResult(interp, listObjPtr);
@@ -13832,24 +13829,18 @@ static int Jim_AppendCoreCommand(Jim_Interp *interp, int argc, Jim_Obj *const *a
1383213829
return JIM_ERR;
1383313830
}
1383413831
else {
13835-
int new_obj = 0;
1383613832
stringObjPtr = Jim_GetVariable(interp, argv[1], JIM_UNSHARED);
1383713833
if (!stringObjPtr) {
1383813834
/* Create the string if it doesn't exist */
1383913835
stringObjPtr = Jim_NewEmptyStringObj(interp);
13840-
new_obj = 1;
1384113836
}
1384213837
else if (Jim_IsShared(stringObjPtr)) {
13843-
new_obj = 1;
1384413838
stringObjPtr = Jim_DuplicateObj(interp, stringObjPtr);
1384513839
}
1384613840
for (i = 2; i < argc; i++) {
1384713841
Jim_AppendObj(interp, stringObjPtr, argv[i]);
1384813842
}
1384913843
if (Jim_SetVariable(interp, argv[1], stringObjPtr) != JIM_OK) {
13850-
if (new_obj) {
13851-
Jim_FreeNewObj(interp, stringObjPtr);
13852-
}
1385313844
return JIM_ERR;
1385413845
}
1385513846
}

‎jim.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -838,6 +838,7 @@ JIM_EXPORT int Jim_RenameCommand (Jim_Interp *interp,
838838
Jim_Obj *oldNameObj, Jim_Obj *newNameObj);
839839
JIM_EXPORT Jim_Cmd * Jim_GetCommand (Jim_Interp *interp,
840840
Jim_Obj *objPtr, int flags);
841+
/* Note that if Jim_SetVariable() fails, and valObjPtr has a zero reference count, it will be freed */
841842
JIM_EXPORT int Jim_SetVariable (Jim_Interp *interp,
842843
Jim_Obj *nameObjPtr, Jim_Obj *valObjPtr);
843844
JIM_EXPORT int Jim_SetVariableStr (Jim_Interp *interp,

‎tests/loop.test‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,15 @@ test loop-2.8 {modify loop var} {
152152
set a
153153
} {1 2 3 4 5}
154154

155+
# Previously this would leak memory (configure --maintainer)
156+
test loop-2.9 {fail to set loop var} -body {
157+
set i 1
158+
loop i(x) 1 6 {
159+
incr y
160+
}
161+
set y
162+
} -returnCodes error -result {can't set "i(x)": variable isn't array}
163+
155164
testreport
156165
break
157166

0 commit comments

Comments
 (0)