Correct logic for handling error cases when setting the namespace unknown handler.

3 files changed, 76 insertions, 52 deletions

diff --git a/ChangeLog b/ChangeLog
index ecbc97f..7820292 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,7 +1,13 @@
+2008-05-20  Donal K. Fellows  <dkf@users.sf.net>
+
+	* generic/tclNamesp.c (Tcl_SetNamespaceUnknownHandler): Corrected odd
+	logic for handling installation of namespace unknown handlers which
+	could lead too very strange things happening in the error case.
+
 2008-05-16  Miguel Sofer  <msofer@users.sf.net>
 
-	* generic/tclCompile.c: fix crash with tcl_traceExec. Found and
-	fixed by Alexander Pasadyn [Bug 1964803]. 
+	* generic/tclCompile.c: fix crash with tcl_traceExec. Found and fixed
+	by Alexander Pasadyn. [Bug 1964803]
 
 2008-05-15  Pat Thoyts  <patthoyts@users.sourceforge.net>
 
@@ -15,38 +21,34 @@
 	extension and tsdPerf.so to get some performance metrics by,
 	simulating, simple TSD contention.
 
-
 2008-05-09  George Peter Staplin  <georgeps@xmission.com>
 
 	* generic/tcl.h: 	Make Tcl_ThreadDataKey a void *.
-	* generic/tclInt.h:	Change around some function names and
-	 add some new per-platform declarations for thread-specific data
-	 functions.
-	* generic/tclThread.c:  Make use of of the new function names
-	 that no longer have a Tclp prefix.	
-	* generic/tclThreadStorage.c: Replace the core thread-specific data 
-	(TSD) mechanism with an array offset solution that eliminates the
-	hash tables, and only uses one slot of native TSD.  
-	Many thanks to Kevin B. Kenny for his help with this.
-
-	* unix/tclUnixThrd.c:	Add platform-specific TSD functions for use
-	by tclThreadStorage.c. 		
-	* win/tclWinThrd.c: 	Add platform-specific TSD functions for use 
-	by tclThreadStorage.c.
+	* generic/tclInt.h:	Change around some function names and add some
+	new per-platform declarations for thread-specific data functions.
+	* generic/tclThread.c:  Make use of of the new function names that no
+	longer have a Tclp prefix.
+	* generic/tclThreadStorage.c: Replace the core thread-specific data
+	(TSD) mechanism with an array offset solution that eliminates the hash
+	tables, and only uses one slot of native TSD. Many thanks to Kevin B.
+	Kenny for his help with this.
+
+	* unix/tclUnixThrd.c:	Add platform-specific TSD functions for use by
+	tclThreadStorage.c.
+	* win/tclWinThrd.c: 	Add platform-specific TSD functions for use by
+	tclThreadStorage.c.
 
 2008-05-09  Kevin B. Kenny  <kennykb@acm.org>
 
-	* tests/dict.test (dict-19.2): Corrected a bug where
-	the test was changed to use [apply] instead of a temporary
-	proc, but the cleanup script still attempted to delete
-	the temporary proc.
-	
+	* tests/dict.test (dict-19.2): Corrected a bug where the test was
+	changed to use [apply] instead of a temporary proc, but the cleanup
+	script still attempted to delete the temporary proc.
+
 2008-05-07  Donal K. Fellows  <dkf@cspool38.cs.man.ac.uk>
 
-	* generic/tclCompCmds.c (TclCompileDictAppendCmd): Fix silly
-	off-by one error that caused a crash every time a compiled 'dict
-	append' with more than one argument was used. Found by Colin
-	McCormack.
+	* generic/tclCompCmds.c (TclCompileDictAppendCmd): Fix silly off-by
+	one error that caused a crash every time a compiled 'dict append' with
+	more than one argument was used. Found by Colin McCormack.
 
 2008-05-02  Pat Thoyts  <patthoyts@users.sourceforge.net>
 
diff --git a/generic/tclNamesp.c b/generic/tclNamesp.c
index f7fa9c1..691c62f 100644
--- a/generic/tclNamesp.c
+++ b/generic/tclNamesp.c
@@ -23,7 +23,7 @@
  * See the file "license.terms" for information on usage and redistribution of
  * this file, and for a DISCLAIMER OF ALL WARRANTIES.
  *
- * RCS: @(#) $Id: tclNamesp.c,v 1.162 2008/03/02 18:46:39 msofer Exp $
+ * RCS: @(#) $Id: tclNamesp.c,v 1.163 2008/05/20 22:22:16 dkf Exp $
  */
 
 #include "tclInt.h"
@@ -4296,45 +4296,58 @@ Tcl_SetNamespaceUnknownHandler(
     Tcl_Namespace *nsPtr,	/* Namespace which is being updated. */
     Tcl_Obj *handlerPtr)	/* The new handler, or NULL to reset. */
 {
-    int lstlen;
+    int lstlen = 0;
     Namespace *currNsPtr = (Namespace *)nsPtr;
 
-    if (currNsPtr->unknownHandlerPtr != NULL) {
-	/*
-	 * Remove old handler first.
-	 */
+    /*
+     * Ensure that we check for errors *first* before we change anything.
+     */
 
-	Tcl_DecrRefCount(currNsPtr->unknownHandlerPtr);
-	currNsPtr->unknownHandlerPtr = NULL;
+    if (handlerPtr != NULL) {
+	if (TclListObjLength(interp, handlerPtr, &lstlen) != TCL_OK) {
+	    /*
+	     * Not a list.
+	     */
+
+	    return TCL_ERROR;
+	}
+	if (lstlen > 0) {
+	    /*
+	     * We are going to be saving this handler. Increment the reference
+	     * count before decrementing the refcount on the previous handler,
+	     * so that nothing strange can happen if we are told to set the
+	     * handler to the previous value.
+	     */
+
+	    Tcl_IncrRefCount(handlerPtr);
+	}
     }
 
     /*
-     * If NULL or an empty list is passed, then reset to the default
-     * handler.
+     * Remove old handler next.
      */
 
-    if (handlerPtr == NULL) {
-	currNsPtr->unknownHandlerPtr = NULL;
-    } else if (TclListObjLength(interp, handlerPtr, &lstlen) != TCL_OK) {
-	/*
-	 * Not a list.
-	 */
+    if (currNsPtr->unknownHandlerPtr != NULL) {
+	Tcl_DecrRefCount(currNsPtr->unknownHandlerPtr);
+    }
 
-	return TCL_ERROR;
-    } else if (lstlen == 0) {
+    /*
+     * Install the new handler.
+     */
+
+    if (lstlen > 0) {
 	/*
-	 * Empty list - reset to default.
+	 * Just store the handler. It already has the correct reference count.
 	 */
 
-	currNsPtr->unknownHandlerPtr = NULL;
+	currNsPtr->unknownHandlerPtr = handlerPtr;
     } else {
 	/*
-	 * Increment ref count and store. The reference count is decremented
-	 * either in the code above, or when the namespace is deleted.
+	 * If NULL or an empty list is passed, this resets to the default
+	 * handler.
 	 */
 
-	Tcl_IncrRefCount(handlerPtr);
-	currNsPtr->unknownHandlerPtr = handlerPtr;
+	currNsPtr->unknownHandlerPtr = NULL;
     }
     return TCL_OK;
 }
diff --git a/tests/namespace.test b/tests/namespace.test
index e445189..9fb2d9a 100644
--- a/tests/namespace.test
+++ b/tests/namespace.test
@@ -11,7 +11,7 @@
 # See the file "license.terms" for information on usage and redistribution
 # of this file, and for a DISCLAIMER OF ALL WARRANTIES.
 #
-# RCS: @(#) $Id: namespace.test,v 1.70 2007/12/13 15:26:06 dgp Exp $
+# RCS: @(#) $Id: namespace.test,v 1.71 2008/05/20 22:22:17 dkf Exp $
 
 if {[lsearch [namespace children] ::tcltest] == -1} {
     package require tcltest 2
@@ -2619,7 +2619,16 @@ test namespace-52.11 {unknown: with TCL_EVAL_INVOKE} -setup {
     rename unknown.save ::unknown
     namespace eval :: [list namespace unknown $handler]
 } -result SUCCESS
-    
+test namespace-52.12 {unknown: error case must not reset handler} -body {
+    namespace eval foo {
+	namespace unknown ok
+	catch {namespace unknown {{}{}{}}}
+	namespace unknown
+    }
+} -cleanup {
+    namespace delete foo
+} -result ok
+
 # cleanup
 catch {rename cmd1 {}}
 catch {unset l}