Tk Source Code

$ wish
% pack [scale .s]
% set fooie 5.79e99
5.79e99
% .s config -variable fooie
*** buffer overflow detected ***: wish terminated
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x70bcb)[0x7f29259aabcb]
/lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x37)[0x7f2925a33227]
/lib/x86_64-linux-gnu/libc.so.6(+0xf7360)[0x7f2925a31360]
/lib/x86_64-linux-gnu/libc.so.6(+0xf6919)[0x7f2925a30919]
/lib/x86_64-linux-gnu/libc.so.6(_IO_default_xsputn+0xac)[0x7f29259aebdc]
/lib/x86_64-linux-gnu/libc.so.6(+0x4be81)[0x7f2925985e81]
/lib/x86_64-linux-gnu/libc.so.6(_IO_vfprintf+0xf32)[0x7f2925981c32]
/lib/x86_64-linux-gnu/libc.so.6(__vsprintf_chk+0x8c)[0x7f2925a309ac]
....
7f2927dad000-7f2927dae000 rw-p 00024000 08:08 3014738                    /lib/x86_64-linux-gnu/ld-2.24.so
7f2927dae000-7f2927daf000 rw-p 00000000 00:00 0 
7ffc9d5d4000-7ffc9d5f5000 rw-p 00000000 00:00 0                          [stack]
7ffc9d5fa000-7ffc9d5fc000 r--p 00000000 00:00 0                          [vvar]
7ffc9d5fc000-7ffc9d5fe000 r-xp 00000000 00:00 0                          [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
Aborted
$
Haven't bothered to include whole stack trace as it is so easy to reproduce
This was on Linux, same thing happens on Mac OS but not Windows

Merged to core-8-6-branch and trunk

I have now checked that the following script:

package require Tk
pack [scale .s]
.s configure -from -6.8e99
.s configure -to 8.8e99
set fooie 5.79e99
.s config -variable fooie

does no longer crash, on Windows, Linux, and OS X.

Also the scale.test runs without failure (including the added scale-21.[1|2] tests) on all the above three platforms.

New test added to check non-crashing with huge values for -from and -to.

See [e7902b5a] in branch bug-55b95f578a

Actually there are more places in the scale widget where similar issues occur. For instance consider the following snipped:

package require Tk
pack [scale .s]
.s configure -from -6.8e99
.s configure -to 8.8e99

As mentioned by dgp three years ago in ticket [3417012]:

Longer term, Tk ought get out of the practice of using sprintf(), and if Tcl doesn't offer a public routine to replace it, it should.
(Tcl_PrintDouble() isn't quite right, due to the impact of tcl_precision.)
For now, though, segfaults are bad, and any fix is better than keeping one around.

I decided to go for patch #2 below, see [794a08b2] in branch bug-55b95f578a.

The old and new values are compared as strings formatted by Tcl_PrintDouble. The printed strings are guaranteed to fit in their TCL_DOUBLE_SPACE size (since they are printed by Tcl_PrintDouble). There is no need to use the scalePtr->format parameter, Tcl_PrintDouble does the job.

I confirm that backing out [2df13bc796] revives [891141ffff]. To demonstrate this, run the demo script "test.tcl" provided with [891141ffff], click on the movable slider of the scale (to give focus to the scale widget), and then move the mouse in and out the scale --> myCount increases. This does not happen with the fix [2df13bc796].

I have prompted dgp (author of the fix [2df13bc796] of [891141ffff]) for comments in the chat. His proposal was to backout [2df13bc796] since Tcl numbers obey EIAS since 8.5.0 and [891141ffff] could have been a bug rooted in string round-tripping leading to a different value, which should no longer be an issue.

But backing out [2df13bc796] revives [891141ffff], so this is not an option.

I have added a non-regression testcase in branch bug-55b95f578a

Buffer overrun is as follows:

tkScale.c:678
        char varString[TCL_DOUBLE_SPACE]
        sprintf(varString, scalePtr->format, varValue);

So this is a mismatch between the size of the string produced by sprintf, which can be very long due to the format parameter, and the size of the allocated buffer (buffers using TCL_DOUBLE_SPACE are supposed to be filled in by Tcl_PrintDouble, not by sprintf).

I believe this issue got created by [2df13bc796] which fixed [891141ffff]

I think we can fix this by one of the following two patches:

Patch 1:

Index: generic/tkScale.c
==================================================================
--- generic/tkScale.c
+++ generic/tkScale.c
@@ -19,10 +19,14 @@

 #include "default.h"
 #include "tkInt.h"
 #include "tkScale.h"

+#if defined(_WIN32)
+#define snprintf _snprintf
+#endif
+
 /*
  * The following table defines the legal values for the -orient option. It is
  * used together with the "enum orient" declaration in tkScale.h.
  */

@@ -675,11 +679,11 @@
                valuePtr, &varValue) != TCL_OK)) {
            ScaleSetVariable(scalePtr);
        } else {
            char varString[TCL_DOUBLE_SPACE], scaleString[TCL_DOUBLE_SPACE];

-           sprintf(varString, scalePtr->format, varValue);
-           sprintf(scaleString, scalePtr->format, scalePtr->value);
+           snprintf(varString, TCL_DOUBLE_SPACE, scalePtr->format, varValue);
+           snprintf(scaleString, TCL_DOUBLE_SPACE, scalePtr->format, scalePtr->value);
            if (strcmp(varString, scaleString)) {
                ScaleSetVariable(scalePtr);
            }
        }

Patch 2:

Index: generic/tkScale.c
==================================================================
--- generic/tkScale.c
+++ generic/tkScale.c
@@ -675,12 +675,12 @@
                valuePtr, &varValue) != TCL_OK)) {
            ScaleSetVariable(scalePtr);
        } else {
            char varString[TCL_DOUBLE_SPACE], scaleString[TCL_DOUBLE_SPACE];

-           sprintf(varString, scalePtr->format, varValue);
-           sprintf(scaleString, scalePtr->format, scalePtr->value);
+            Tcl_PrintDouble(NULL, varValue, varString);
+            Tcl_PrintDouble(NULL, scalePtr->value, scaleString);
            if (strcmp(varString, scaleString)) {
                ScaleSetVariable(scalePtr);
            }
        }
        Tcl_TraceVar2(interp, Tcl_GetString(scalePtr->varNamePtr),

I believe patch 1 is better because even if it limits the number of digits in the string rep to compare, it still uses the format parameter, which is not the case for patch 2.

A third approach, that would iteratively resize the buffer each time snprintf returns -1 (insufficient space), seems to be an overkill to me.

Thanks for the report, I see what's wrong here. There is a buffer overrun, and I can see it on Windows as well. TCL_DOUBLE_SPACE is used to dimension a buffer that is populated by sprintf with a format parameter "%.0f", and with the huge number given the string rep of this number does not fit in TCL_DOUBLE_SPACE characters.