Tk Source Code

Here is a double free bug that someone originally reported for Perl/Tk (https://rt.cpan.org/Public/Bug/Display.html?id=102648), but which I replicated in Tcl/Tk using both the original Perl/Tk example through Tcl::pTk, as well as a nearly equivalent Tcl syntax example:

package require Tk

set en_text {Type 'A' here}
pack [entry .e \
    -textvariable en_text \
    -validate key \
    -validatecommand {
        if {"%S" eq {A}} {
            set en_text %P
            .e configure -validate key
        }
        return 1
    }
]

Pressing 'A' causes the double free in this example. When the key is pressed, InsertChars() stores the existing entryPtr->string in the variable string. It then does the EntryValidateChange() → EntryValidate() to evaluate the validatecommand script. When the textvariable is set in the script, this invokes EntryTextVarProc() → EntrySetValue() which frees the existing entryPtr->string. After validation finishes, InsertChars() then tries to free the already-freed address in string.

It's not obvious to me what should be done about this issue. Should this at least not lead to a crash/corruption, or is it believed that the documentation already firmly warned against mixing -textvariable and -validatecommand such that this issue is expected behavior? At a minimum, it preferably crashes rather than silently corrupts—i.e. panic if string != entryPtr->string just before ckfree((char *)string).

I am not aware of this issue affecting Ttk entry widgets.

Merged in core-8-6-branch and trunk.

I have made two additional commits in the bugfix branch, in order to let entry-19.19 pass again (I have changed the expected test result) and to test the spinbox too.

I will merge soon.

The fix consists in properly aborting recursive validation while we're already validating. Such a situation happens when running the validatecommand script (the "outer" validation request): when this script writes the textvariable, a write trace fires and requests validation (the "inner" validation). This inner validation needs to be aborted to prevent the double free from happening. This is done by setting entryPtr->validate to VALIDATE_NONE, which was already implemented (it prevents *next* validations from happening), but before the fix the *current* validation wasn't aborted: entryPtr->flags didn't contain VALIDATE_ABORT.

Now when the textvariable is written more than once the second (and later) time the return path in EntryValidateChange is not the same due to entryPtr->validate being VALIDATE_NONE as set by the first time the textvariable was written. It was my complementary fix to abort at this place also. I don't think there is something more to fix elsewhere but I'm open to be contradicted, of course.

As for the expected behavior of the original example script: the validatecommand script is evaluated, and after that the write traces fire. So the validatecommand script sets the textvariable and sets -validate to "key". Then the write traces fire, it tries to validate the textvariable value set previously, and sets the -validate configuration option to none. So you have the impression that .e configure -validate key has no effect (which is true in fact, but it's because at the time of evaluation, the -validate configuration option is already, and still, "key"). To get what the author of the script could have been after, the following could be used:

package require Tk
set en_text {Type 'A' here}
pack [entry .e \
    -textvariable en_text \
    -validate key \
    -validatecommand {
        if {"%S" eq {A}} {
            set en_text %P
            after idle {.e configure -validate key}
        }
        return 1
    }
]

Finally, yes, the statement from the documentation you're mentioning still applies. Just see results of test entry-19.20 or similar tests. Or even with the original script:

% package require Tk
8.6.10
% set en_text {Type 'A' here}
Type 'A' here
% pack [entry .e \
    -textvariable en_text \
    -validate key \
    -validatecommand {
        if {"%S" eq {A}} {
            set en_text %P
            .e configure -validate key
        }
        return 1
    }
]
% set en_text
Type 'A' here
% # Type letter 'A' in the entry
% set en_text
TyApe 'A' here
% # The entry still shows "Type 'A' here", which is different from $en_text

But, really, the manual should be followed:

the -textvariable and -validatecommand options can be dangerous to mix

I no longer observe the double free, but I do not understand the code well enough to exhaustively say there's no way for one to happen.

I'm also not sure I completely understand the example's expected behavior, and whether other bugs are present. For example, the .e configure -validate key statement in the validatecommand has no effect: validation will always happen up until the textvariable is set, at which point validation will no longer happen, even with .e configure -validate key.

I also don't know if this statement from the documentation still applies:

It is also recommended to not set an associated -textvariable during validation, as that can cause the entry widget to become out of sync with the -textvariable.

How perverse!

My fix [d9ce9e56] sets the VALIDATE_ABORT flag in case a validation loop is detected, i.e. when validation is requested (by a write trace on the textvariable) while we are already validating (from inside the validatecommand, with entryPtr->validate being different from VALIDATE_NONE). This resets entryPtr->validate to VALIDATE_NONE, and prevents further validation steps.

Still while executing the validatecommand, when the textvariable is written a second (or more) time, the VALIDATE_ABORT flag is not set because entryPtr->validate is VALIDATE_NONE this time (the code path is slightly different), even if we're still in the validatecommand.

This is now fixed by [43bc0e3f].

All entry.test tests still pass, including the new entry-19.21, except entry-19.19 which fails as stated previously.

My bad, I was not using the original example. Rather I was using a modified one that sets the textvariable repeatedly:

set en_text {Type 'A' here}
pack [entry .e \
    -textvariable en_text \
    -validate key \
    -validatecommand {
        if {"%S" eq {A}} {
            for {set j 0} {$j < 100} {incr j} {
                set en_text $j
            }
            .e configure -validate key
        }
        return 1
    }
]

Really? With [d9ce9e5659] I don't.

When typing "A" in the entry, line 2376 (the first free) in EntrySetValue() is not executed because EntrySetValue() returns at line 2371. The second free at line 2073 in InsertChars() is also not executed since InsertChars() returns before that point at line 2069.

Under [d9ce9e5659] I still observe the double free issue using the example script.

My original fix proposal was just a bad ugly hack.

I have now something much better, please see [d9ce9e5659]. This fix aborts validation happening during the write trace (that is triggered because the validatecommand script changed the textvariable). EntryTextVarProc() → EntrySetValue() → EntryValidateChange() returns early, preventing freeing of the existing entryPtr->string, and therefore suppressing the double free.

The newly added test entry-19.21 passes with this fix (and fails without).

All other tests from entry.test still pass, except entry-19.19 which newly fails as follows:

---- Result was:
none nextdata {.e -1 -1 nextdata nextdata {} all forced}
---- Result should have been (exact matching):
none mydata {.e -1 -1 nextdata nextdata {} all forced}
==== entry-19.19 FAILED

This change in entry-19.19 results may well be fine since this test exercises one of the "dangerous" conditions highlighted in the man page (one of the "don't do that" cases).

What do you think?

Thanks for fixing the function names in the description.

It has occurred to me that, by assigning to the textvariable multiple times in the validatecommand, it might be possible for a subsequent call to EntrySetValue() to reobtain the (already-freed) address in string in InsertChars(). I have not managed to observe this, but I haven't ruled it out either; if it is possible then the proposed fix might not be sufficient.

(Preliminary note: I have a bit edited the report since I think it contained wrong names of called functions at threee places.)

About a possible fix, wouldn't the following patch be okay:

Index: generic/tkEntry.c
==================================================================
--- generic/tkEntry.c
+++ generic/tkEntry.c
@@ -2067,11 +2067,13 @@
 		    VALIDATE_INSERT) != TCL_OK) {
 	ckfree(newStr);
 	return TCL_OK;
     }
 
-    ckfree((char *)string);
+    if (string == entryPtr->string) {
+        ckfree((char *)string);
+    }
     entryPtr->string = newStr;
 
     /*
      * ??? Is this construction still needed with Tcl_NumUtfChars ???
      *

Indeed, checking whether or not string is still the same as entryPtr->string at this point gives a way to detect that string was already freed. It happens, as you said, because the linked textvariable is updated by the validation script. I think there can be no other reason for string to have been already freed (what do you think?), so this patch should fix the crash without regression.

Of course, the above patch should be augmented with adequate comments to explain what's going on since it's not exactly straightforward.