#2901 Buffer overrun in [subst]

[Donal K. Fellows]

This is not a problem in the HEAD.

Repeatedly do this command. The error result is always
strange and occasionally it crashes. (Seems random which.)
subst {[set x 1;}

GDB claims crash happened like this:
Program received signal SIGSEGV, Segmentation fault.
0x0807e5ab in Tcl_SubstObj (interp=0x8103390,
objPtr=0x810bd10, flags=7)
at ../generic/tclCmdMZ.c:2578
2578 switch (*p) {
The 'p' pointer seems to have overflowed well past the
end of the objPtr->bytes parameter:
(gdb) print p
$1 = 0x812c000 <Address 0x812c000 out of bounds>
(gdb) print objPtr->bytes
$2 = 0x8127788 "[set x 1;"

Looks like a buffer overrun to me, and [subst] is often
exposed to external sources. => CRITICAL BUG

[miguel sofer]

Logged In: YES
user_id=148712

The problem seems to be in Tcl_EvalEx: it fails when it
evaluates a nested script of unknown length (numBytes==-1)
that is missing the closing ']' AND ends in ';' (a space
after that seems not to trigger the buffer overflow).

In any case, the attached patch to core-8-4-branch seems to
fix it. There might be a more elegant to fix this ...

Note that Tcl_EvalEx has been rewritten in HEAD; I did not
yet check if the problemmight be there too.

[Donal K. Fellows]

Logged In: YES
user_id=79902

Syntax error is reported wrongly. :^(

When I did the following after applying your patch:
subst "\[subst {};"
I got this error:
can't read "f": no such variable

[miguel sofer]

Logged In: YES
user_id=148712

updated patch - still missing tests

[miguel sofer]

[Donal K. Fellows]

Logged In: YES
user_id=79902

Fix looks good. Suggested test (you might want to give it a
different test number):
test subst-12.1 {nasty case} {
list [catch {subst "\[subst {};"} msg] $msg
} {1 {missing close-bracket}}

[miguel sofer]

Logged In: YES
user_id=148712

Fix and tests committed to core-8-4-branch.

Still to do: commit the new tests to HEAD, check if HEAD has
the flaw too.

[miguel sofer]

Logged In: YES
user_id=148712

tests added to HEAD; code review not yet done

[Don Porter]

Logged In: YES
user_id=80530

added more tests (subst-12.3,4)
to show the effect of syntax errors
on side effects.

Test subst-12.3 illustrates a change
in behavior from 8.4 to 8.5.

[Don Porter]

Logged In: YES
user_id=80530

corrected HEAD to have same
subst-12.3 results as the 8.4 branch.

At this point [subst] does not crash
and is consistent between 8.4 and 8.5..
Anyone wanted other mods to [subst]
should file a separate report.