(Backport) Fix [Bug 2891556] and improve test to detect similar manifestations in the future.

3 files changed, 29 insertions, 7 deletions

diff --git a/ChangeLog b/ChangeLog
index 40bda77..981328b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2009-11-16  Alexandre Ferrieux  <ferrieux@users.sourceforge.net>
+
+	* generic/tclEncoding.c: (Backport) Fix [Bug 2891556] and improve
+	* tests/econding.test:   test to detect similar manifestations in the future.
+	
 2009-11-12  Andreas Kupries  <andreask@activestate.com>
 
 	* generic/tclIO.c (CopyData): [Bug 2895565]. Dropped bogosity
diff --git a/generic/tclEncoding.c b/generic/tclEncoding.c
index 99c71b0..843d772 100644
--- a/generic/tclEncoding.c
+++ b/generic/tclEncoding.c
@@ -8,7 +8,7 @@
  * See the file "license.terms" for information on usage and redistribution
  * of this file, and for a DISCLAIMER OF ALL WARRANTIES.
  *
- * RCS: @(#) $Id: tclEncoding.c,v 1.16.2.15 2008/07/04 19:21:19 jenglish Exp $
+ * RCS: @(#) $Id: tclEncoding.c,v 1.16.2.16 2009/11/16 17:03:29 ferrieux Exp $
  */
 
 #include "tclInt.h"
@@ -575,6 +575,9 @@ FreeEncoding(encoding)
     if (encodingPtr == NULL) {
 	return;
     }
+    if (encodingPtr->refCount<=0) {
+	Tcl_Panic("FreeEncoding: refcount problem !!!");
+    }
     encodingPtr->refCount--;
     if (encodingPtr->refCount == 0) {
 	if (encodingPtr->freeProc != NULL) {
@@ -3003,11 +3006,24 @@ EscapeFreeProc(clientData)
     if (dataPtr == NULL) {
 	return;
     }
-    subTablePtr = dataPtr->subTables;
-    for (i = 0; i < dataPtr->numSubTables; i++) {
-	FreeEncoding((Tcl_Encoding) subTablePtr->encodingPtr);
-	subTablePtr++;
-    }
+    /*
+     *  The subTables should be freed recursively in normal operation but not
+     *  during TclFinalizeEncodingSubsystem because they are also present as a
+     *  weak reference in the toplevel encodingTable (ie they don't have a +1
+     *  refcount for this), and unpredictable nuking order could remove them
+     *  from under the following loop's feet [Bug 2891556].
+     *  
+     *  The encodingsInitialized flag, being reset on entry to TFES, can serve
+     *  as a "not in finalization" test.
+     */
+    if (encodingsInitialized)
+	{
+	    subTablePtr = dataPtr->subTables;
+	    for (i = 0; i < dataPtr->numSubTables; i++) {
+		FreeEncoding((Tcl_Encoding) subTablePtr->encodingPtr);
+		subTablePtr++;
+	    }
+	}
     ckfree((char *) dataPtr);
 }
 
diff --git a/tests/encoding.test b/tests/encoding.test
index 4e5a7fa..22d5c9b 100644
--- a/tests/encoding.test
+++ b/tests/encoding.test
@@ -8,7 +8,7 @@
 # See the file "license.terms" for information on usage and redistribution
 # of this file, and for a DISCLAIMER OF ALL WARRANTIES.
 #
-# RCS: @(#) $Id: encoding.test,v 1.16.2.3 2006/10/05 21:24:56 hobbs Exp $
+# RCS: @(#) $Id: encoding.test,v 1.16.2.4 2009/11/16 17:03:29 ferrieux Exp $
 
 package require tcltest 2
 namespace import -force ::tcltest::*
@@ -402,6 +402,7 @@ test encoding-24.2 {EscapeFreeProc on open channels} -constraints {
 } -setup {
     # Bug #524674 output
     set file [makeFile {
+	encoding system cp1252;	# Bug #2891556 crash revelator
 	fconfigure stdout -encoding iso2022-jp
 	puts ab\u4e4e\u68d9g
 	exit