Tcl Source Code

We ran into a problem in BRL-CAD with the maximum command length in Tcl and have a fix that extends the maximum command limit approximately two times where it presently fails.  The provided patch pushes that limit up about 2x by checking for the overflow and clamping to maxint.  If that maxint limit is exceeded, then it now returns -1 instead of crashing too.

The current behavior without the patch is that 'length' ends up overflowing causing a crash inside Tcl_SetObjLength() where it tries to use length (which is negative maxint) as an array index.

For those wondering just how we'd actually blow out that limit, our (.asc) ASCII file format in BRL-CAD is essentially a Tcl transcript.  So to load a file, we just have Tcl parse the file.  Being a CAD package, our files are geometry.  One of the geometry types is a polygonal mesh... So when you have one mesh that is really big (which is really easy to do), you end up blowing out the command length.

This fix should be credited to Bob Parker (BRL-CAD developer) if it is applied.  The patch was made against 8.5.1

We fully admit that the patch isn't ideal, but it does resolve the problem from our standpoint without any negative consequences until the API can be improved.  As you're undoubtedly well-aware, DoReadChars() and friends need to not use int everywhere for string/buffer length tracking, instead using size_t's or something similarly longer (and fixed) instead of int.

Cheers!
Sean

Thanks for the explanation. It makes sense.

allow_comments - 1

accepted for 8.5.8 and 8.6b2.

The problem to avoid is when the "length" value
passed to Tcl_(Attempt)SetObjLength no longer
really represents what the caller desires because
of integer overflow.  The really trouble some case
is when offset is already INT_MAX.  Then the
doubling rule led to an attempt to allocate a new
string of length 2*offset + TCL_UTF_MAX + 1,
which wraps around the unsigned integer range
and allocates a string of length 2.  Then later
code tries to write long strings into a buffer of length
2 and segfaults.

Shifting the location of the TCL_UTF_MAX offset
in the calculation means that the calculation of
length is limited to 2*offset, which can be no bigger
that UINT_MAX -1, which when interpreted as a signed
int is negative, and triggers the appropriate failure.

The whole TCL_UTF_MAX shifting business is needed
to satisfy the demands of the Tcl_ExternalToUtf() routine.

The +1 that used to be there was apparently in place to
make sure that space for a terminating NUL was allocated.
Since the Tcl_*SetObjLength() routines already provide
for this, it was removed.

dgp, the patch looks ok to me in general. Even if I do not fully understand the exact tweaks re dstNeeded and/or spaceLeft, with the TCL_UTF_MAX moving between them.

Second version of the patch adds similar fixes
to FilterInputBytes().

File Added - 346764: 2107634-2.patch

Attached a patch that better governs buffer growth so that
ReadChars can keep reading chars up until the read that
will overflow the limits of Tcl values.  On that read attempt,
there will be a panic.

Please give the patch a try and see if it solves the BRL-CAD
problems at least as well as the contributed patch.

If there is some value in having ReadChars() return -1 rather
than panic when the actual limits are reached, please explain
in a followup comment.

File Added - 346653: 2107634.patch

In the submitted patch, when a read tries to
go beyond the capacity of a Tcl_Value,
ReadChars() returns -1.  What does that do?

Related issue Bug 2418583 indicates
that overflows with Tcl_ExternalToUtf() 
also need attention.

Your patch appears to be preventing the
passing of a negative length value to
Tcl_SetObjLength().  This means the new
T_SOL behavior of panic on a negative
length will be avoided by your patch.  The
next releases should not harm your
patch's effectiveness.

Leaving this open, since the failure of
the caller(s) of T_SOL to take notice
of and react to string length overflow
remains a problem, and the development
of a better patch to go into the core
sources still needs doing.

Thanks for the heads up.  We'll have to evaluate/merge the change if we update our bundled version of Tcl/Tk too, so it's good to know.  If/when we do before you examine the impact on your end, I'll update the patch.

Fixing another bug (2553906) we've recently
added checks to Tcl_(Attempt)SetObjLength to
reject negative length requests.  I'll try
to find time to examine what impact that
might have on this patch.

File Added - 293150: cmd_2x_bug.patch