Tcl Source Code

The refCount management of TclPtrSetVar, and also its
different "front ends" in tclVar.c, presents one
particular problem which was revealed by [Bug 1191369]
(both 8.4 and 8.5 are concerned).

The problem arises when the newValuePtr obj has refCount 0:

(1) if setting the new value succeeds as requested,
newValuePtr gets its refCount incremented to reflect
the new reference

(2) if setting the new value fails early (eg, trying to
set an array to a scalar value), newValuePtr's refCount
is not touched 

(3) If there is a write trace that does not allow the
value to be set as requested, newValuePtr's refCount
will be incremented/decremented, resulting in it being
freed. 

This renders the caller's refCount management rather
critical, as reflected in 1191369. There are other
spots in the core where a similar bug is present.
Example: Tcl_AddObjErrorInfo would leak an obj if
errorCode or errorInfo redefined to be arrays (I know,
well deserved); Tcl_Main would leak if "argc" was an
array. Worse, witness (in a MEM_DEBUG HEAD):

% trace add variable a write {return -code error "RO
var";#}
% regexp qw qwerty -> a
couldn't set variable "a"
% set a
file = ../generic/tclExecute.c, line = 2099
Trying to increment refCount of previously disposed object.


There are two possible fixes:
 
(a) fix all callers so that they incr before calling,
decr on return. Add a warning in the docs.

(b) fix TclPtrSetVar so that it does incr/decr for all
paths where it fails to set the value as requested,
allowing the callers to delegate the refCount
management to TclPtrSetVar. The callers still have to
be fixed to not touch that refCount.

I have a preference for (b); assigning to dgp as a
request for opinion. Please comment and assign back.

allow_comments - 1

...and trunk.

Calling TclFreeObj() on the same value
twice causes really severe damage.  At
least in TCL_MEM_DEBUG mode we 
should better detect and report this.  This
will help a great deal finding the next bug
of this same kind, if any remain.

Patch committed to 8.5 branch.

This patch changed the refcount demands of
several variable setting routines.  The new
interface contract is much nicer, but some existing
code was written to the old requirements, and
hasn't yet been updated to the new reality.  This
code now breaks in ugly ways.

This bug fix causes segfaults in Tk.
See Tk Bug 3607326.

File Added - 155064: 1334947-8-5.patch

Logged In: YES 
user_id=148712

Fixed in HEAD by method (b).

File Added - 153548: 1334947-8-4.patch

Logged In: YES 
user_id=148712

Fixed in 8.4 by method (a). Attempting (b) caused many
testsuite failures, a fix was deemed too risky as many
extensions may have copied the buggy core code. Patch
attached to this ticket.

HEAD still untouched.

Logged In: YES 
user_id=68433

FWIW, I agree with Miguel that option (b) is strongly
preferable.  Callers should be able to pass a fresh Tcl_Obj*
to Tcl_ObjSetVar() and relatives without having to increment
/ decrement the refcount before and after.

Logged In: YES 
user_id=148712

Clarifying my thoughts: the real "bug" is that some failure
paths do not clear 0-ref objs, some do not. This makes it
imperative for callers to manage the refCount explicitly. 

If a new obj is created by the caller and used as new value
without incrementing its refCount, the handling of error
returns is impossible to get right:either you DecrRefCount
and crash on errors caused by traces, or you don't and leak
the obj on other errors.

My preference for alternative (b) is that it allows the
fire-and-forget technique: just call e.g. 
  Tcl_ObjSetVar2(interp, part1Ptr, part2Ptr, Tcl_NewObj(),
flags)
and forget all about that object's fate in the knowledge
that it will be disposed of proeprly (as is currently done
in  error by eg Tcl_AddObjErrorInfo).

Maybe a similar policy would make sense for other
interfaces, eg Tcl_ListObjAppendElement?

Logged In: YES 
user_id=148712

Better crash example:
% trace add variable a write {set a 1; return -code error
"RO var";#}
% regexp qw qwerty -> a
file = ../generic/tclCmdMZ.c, line = 370
Trying to decrement refCount of previously disposed object.

Logged In: YES 
user_id=148712

correction to (b): either not touch it, or else incr/decr on
all paths.