closes [79474c58800cdf94]: fixes 2 segfaults and 2 leaks (common IO handlers and reflected channels)

3 files changed, 179 insertions, 41 deletions

diff --git a/generic/tclIO.c b/generic/tclIO.c
index 6b77749..165a07e 100644
--- a/generic/tclIO.c
+++ b/generic/tclIO.c
@@ -97,6 +97,7 @@ typedef struct GetsState {
 typedef struct CopyState {
     struct Channel *readPtr;	/* Pointer to input channel. */
     struct Channel *writePtr;	/* Pointer to output channel. */
+    int refCount;		/* Reference counter. */
     int readFlags;		/* Original read channel flags. */
     int writeFlags;		/* Original write channel flags. */
     Tcl_WideInt toRead;		/* Number of bytes to copy, or -1. */
@@ -219,6 +220,7 @@ static int		StackSetBlockMode(Channel *chanPtr, int mode);
 static int		SetBlockMode(Tcl_Interp *interp, Channel *chanPtr,
 			    int mode);
 static void		StopCopy(CopyState *csPtr);
+static void		CopyDecrRefCount(CopyState *csPtr);
 static void		TranslateInputEOL(ChannelState *statePtr, char *dst,
 			    const char *src, int *dstLenPtr, int *srcLenPtr);
 static void		UpdateInterest(Channel *chanPtr);
@@ -2081,7 +2083,7 @@ Tcl_UnstackChannel(
 		return TCL_ERROR;
 	    }
 
-	    statePtr->csPtrR  = csPtrR;
+	    statePtr->csPtrR = csPtrR;
 	    statePtr->csPtrW = csPtrW;
 	}
 
@@ -2947,6 +2949,34 @@ FlushChannel(
     return errorCode;
 }
 
+static void
+FreeChannelState(
+    char *blockPtr)		/* Channel state to free. */
+{
+    ChannelState *statePtr = (ChannelState *)blockPtr;
+    /*
+     * Even after close some members can be filled again (in events etc).
+     * Test in bug [79474c588] illustrates one leak (on remaining chanMsg).
+     * Possible other fields need freeing on some constellations.
+     */
+
+    DiscardInputQueued(statePtr, 1);
+    if (statePtr->curOutPtr != NULL) {
+	ReleaseChannelBuffer(statePtr->curOutPtr);
+    }
+    DiscardOutputQueued(statePtr);
+    
+    DeleteTimerHandler(statePtr);
+
+    if (statePtr->chanMsg) {
+	Tcl_DecrRefCount(statePtr->chanMsg);
+    }
+    if (statePtr->unreportedMsg) {
+	Tcl_DecrRefCount(statePtr->unreportedMsg);
+    }
+    ckfree(statePtr);
+}
+
 /*
  *----------------------------------------------------------------------
  *
@@ -3123,7 +3153,7 @@ CloseChannel(
 
     ChannelFree(chanPtr);
 
-    Tcl_EventuallyFree(statePtr, TCL_DYNAMIC);
+    Tcl_EventuallyFree(statePtr, FreeChannelState);
 
     return errorCode;
 }
@@ -3941,8 +3971,14 @@ Tcl_ClearChannelHandlers(
      * Cancel any pending copy operation.
      */
 
-    StopCopy(statePtr->csPtrR);
-    StopCopy(statePtr->csPtrW);
+    if (statePtr->csPtrR) {
+	StopCopy(statePtr->csPtrR);
+	statePtr->csPtrR = NULL;
+    }
+    if (statePtr->csPtrW) {
+	StopCopy(statePtr->csPtrW);
+	statePtr->csPtrW = NULL;
+    }
 
     /*
      * Must set the interest mask now to 0, otherwise infinite loops will
@@ -9248,6 +9284,7 @@ TclCopyChannel(
     csPtr->bufSize = !moveBytes * inStatePtr->bufSize;
     csPtr->readPtr = inPtr;
     csPtr->writePtr = outPtr;
+    csPtr->refCount = 2; /* two references below (inStatePtr, outStatePtr) */
     csPtr->readFlags = readFlags;
     csPtr->writeFlags = writeFlags;
     csPtr->toRead = toRead;
@@ -9258,7 +9295,10 @@ TclCopyChannel(
     }
     csPtr->cmdPtr = cmdPtr;
 
-    inStatePtr->csPtrR  = csPtr;
+    TclChannelPreserve(inChan);
+    TclChannelPreserve(outChan);
+
+    inStatePtr->csPtrR = csPtr;
     outStatePtr->csPtrW = csPtr;
 
     if (moveBytes) {
@@ -9272,7 +9312,7 @@ TclCopyChannel(
 
     if ((nonBlocking == CHANNEL_NONBLOCKING) && (toRead == 0)) {
 	Tcl_CreateTimerHandler(0, ZeroTransferTimerProc, csPtr);
-	return 0;
+	return TCL_OK;
     }
 
     /*
@@ -9547,6 +9587,8 @@ CopyData(
 				/* Encoding control */
     int underflow;		/* Input underflow */
 
+    csPtr->refCount++;		/* avoid freeing during handling */
+
     inChan	= (Tcl_Channel) csPtr->readPtr;
     outChan	= (Tcl_Channel) csPtr->writePtr;
     inStatePtr	= csPtr->readPtr->state;
@@ -9667,7 +9709,7 @@ CopyData(
 		    TclDecrRefCount(bufObj);
 		    bufObj = NULL;
 		}
-		return TCL_OK;
+		goto done;
 	    }
 	}
 
@@ -9758,7 +9800,7 @@ CopyData(
 		TclDecrRefCount(bufObj);
 		bufObj = NULL;
 	    }
-	    return TCL_OK;
+	    goto done;
 	}
 
 	/*
@@ -9780,7 +9822,7 @@ CopyData(
 		TclDecrRefCount(bufObj);
 		bufObj = NULL;
 	    }
-	    return TCL_OK;
+	    goto done;
 	}
     } /* while */
 
@@ -9832,6 +9874,9 @@ CopyData(
 	    }
 	}
     }
+
+  done:
+    CopyDecrRefCount(csPtr);
     return result;
 }
 
@@ -9938,8 +9983,6 @@ DoRead(
 	    code = GetInput(chanPtr);
 	    bufPtr = statePtr->inQueueHead;
 
-	    assert(bufPtr != NULL);
-
 	    if (GotFlag(statePtr, CHANNEL_EOF|CHANNEL_BLOCKED)) {
 		/*
 		 * Further reads cannot do any more.
@@ -9948,20 +9991,21 @@ DoRead(
 		break;
 	    }
 
-	    if (code) {
-		/*
-	     * Read error
-	     */
-
-		UpdateInterest(chanPtr);
-		TclChannelRelease((Tcl_Channel)chanPtr);
-		return -1;
+	    if (code || !bufPtr) {
+		/* Read error (or channel dead/closed) */
+		goto readErr;
 	    }
 
 	    assert(IsBufferFull(bufPtr));
 	}
 
-	assert(bufPtr != NULL);
+	if (!bufPtr) {
+	  readErr:
+
+	    UpdateInterest(chanPtr);
+	    TclChannelRelease((Tcl_Channel)chanPtr);
+	    return -1;
+	}
 
 	bytesRead = BytesLeft(bufPtr);
 	bytesWritten = bytesToRead;
@@ -10170,9 +10214,32 @@ StopCopy(
 	Tcl_DeleteChannelHandler(inChan, MBEvent, csPtr);
 	Tcl_DeleteChannelHandler(outChan, MBEvent, csPtr);
 	TclDecrRefCount(csPtr->cmdPtr);
+	csPtr->cmdPtr = NULL;
+    }
+
+    if (inStatePtr->csPtrR) {
+	assert(inStatePtr->csPtrR == csPtr);
+	inStatePtr->csPtrR = NULL;
+	CopyDecrRefCount(csPtr);
+    }
+    if (outStatePtr->csPtrW) {
+	assert(outStatePtr->csPtrW == csPtr);
+	outStatePtr->csPtrW = NULL;
+	CopyDecrRefCount(csPtr);
+    }
+}
+
+static void
+CopyDecrRefCount(
+    CopyState *csPtr)
+{
+    if (csPtr->refCount-- > 1) {
+	return;
     }
-    inStatePtr->csPtrR = NULL;
-    outStatePtr->csPtrW = NULL;
+
+    TclChannelRelease((Tcl_Channel)csPtr->readPtr);
+    TclChannelRelease((Tcl_Channel)csPtr->writePtr);
+
     ckfree(csPtr);
 }
 
diff --git a/generic/tclIORChan.c b/generic/tclIORChan.c
index 727239b..f2bb186 100644
--- a/generic/tclIORChan.c
+++ b/generic/tclIORChan.c
@@ -2211,23 +2211,37 @@ NextHandle(void)
     return resObj;
 }
 
-static void
-FreeReflectedChannel(
-    char *blockPtr)
+static inline void
+CleanRefChannelInstance(
+    ReflectedChannel *rcPtr)
 {
-    ReflectedChannel *rcPtr = (ReflectedChannel *) blockPtr;
-    Channel *chanPtr = (Channel *) rcPtr->chan;
-
-    TclChannelRelease((Tcl_Channel)chanPtr);
     if (rcPtr->name) {
+	/* 
+	 * Reset obj-type (channel is deleted or dead anyway) to avoid leakage
+	 * by cyclic references (see bug [79474c58800cdf94]).
+	 */
+	TclFreeIntRep(rcPtr->name);
 	Tcl_DecrRefCount(rcPtr->name);
+	rcPtr->name = NULL;
     }
     if (rcPtr->methods) {
 	Tcl_DecrRefCount(rcPtr->methods);
+	rcPtr->methods = NULL;
     }
     if (rcPtr->cmd) {
 	Tcl_DecrRefCount(rcPtr->cmd);
+	rcPtr->cmd = NULL;
     }
+}
+static void
+FreeReflectedChannel(
+    char *blockPtr)
+{
+    ReflectedChannel *rcPtr = (ReflectedChannel *) blockPtr;
+    Channel *chanPtr = (Channel *) rcPtr->chan;
+
+    TclChannelRelease((Tcl_Channel)chanPtr);
+    CleanRefChannelInstance(rcPtr);
     ckfree(rcPtr);
 }
 
@@ -2497,18 +2511,7 @@ MarkDead(
     if (rcPtr->dead) {
 	return;
     }
-    if (rcPtr->name) {
-	Tcl_DecrRefCount(rcPtr->name);
-	rcPtr->name = NULL;
-    }
-    if (rcPtr->methods) {
-	Tcl_DecrRefCount(rcPtr->methods);
-	rcPtr->methods = NULL;
-    }
-    if (rcPtr->cmd) {
-	Tcl_DecrRefCount(rcPtr->cmd);
-	rcPtr->cmd = NULL;
-    }
+    CleanRefChannelInstance(rcPtr);
     rcPtr->dead = 1;
 }
 
diff --git a/tests/ioCmd.test b/tests/ioCmd.test
index b8cf52b..74fabe7 100644
--- a/tests/ioCmd.test
+++ b/tests/ioCmd.test
@@ -2117,6 +2117,74 @@ test iocmd-32.2 {delete interp of reflected chan} {
     interp delete child
 } {}
 
+# 1st attempt without error in write, another with error in write:
+foreach ::writeErr {0 1} {
+test iocmd-32.3.$::writeErr {prevent copy-state against segfault by finalize, bug [79474c58800cdf94]} -setup {
+    proc test_chan {args} {
+      set rest [lassign $args mode chan]
+      lappend ::ret $mode
+      switch -exact $mode {
+        read {puts $chan "Test" ; close $chan}
+        write {if {$::writeErr} {return "boom"}; set data [lindex $rest 0]; string length $data}
+        finalize {after 20 {set ::done done}}
+        initialize {return "initialize watch finalize read write"}
+      }
+    }
+    set clchlst {}
+    set toev [after 5000 {set ::done tout}]
+} -body {
+    set ::ret {}
+    set ch [chan create "read write" test_chan]
+    lappend clchlst $ch
+
+    lassign [chan pipe] in1 out1
+    lappend clchlst $in1 $out1
+    lassign [chan pipe] in2 out2
+    lappend clchlst $in2 $out2
+    lassign [chan pipe] in3 out3
+    lappend clchlst $in3 $out3
+
+    # simulate exec: echo test >@ $out2 2>@ $out3 <@ $in1 &:
+    fileevent $out2 writable [list apply {{cho che} {
+	puts $cho test; close $cho; close $che
+    }} $out2 $out3]
+    # recopy to given chans in handler
+    fileevent $in2 readable [list apply {{in out} {
+	if {[catch {
+	    chan copy $in $out
+	} msg]} {
+	    #puts err:$msg
+	    fileevent $in readable {}
+	}
+    }} $in2 $ch]
+    fileevent $in3 readable [list apply {{in out} {
+	if {[catch {
+	    chan copy $in $out
+	} msg]} {
+	    #puts err:$msg
+	    fileevent $in readable {}
+	}
+    }} $in3 $ch]
+    fileevent $out1 writable [list apply {{in out} {
+	if {[catch {
+	    chan copy $in $out
+	} msg]} {
+	    #puts err:$msg
+	    fileevent $out writable {}
+	}
+    }} $ch $out1]
+
+    vwait ::done
+    lappend ::ret $::done
+} -cleanup {
+    foreach ch $clchlst {
+	catch {close $ch}
+    }
+    after cancel $toev
+    unset -nocomplain ::done ::ret ch in1 in2 in3 out1 out2 out3 toev clchlst
+} -result {initialize read write finalize done}
+}; unset ::writeErr
+
 # ### ### ### ######### ######### #########
 ## Same tests as above, but exercising the code forwarding and
 ## receiving driver operations to the originator thread.