From 87f75437c09a8e8fbe5fe0eaa68200c773799e28 Mon Sep 17 00:00:00 2001 From: dgp Date: Thu, 17 Apr 2014 19:58:41 +0000 Subject: Another test exposing another segfault. --- tests/ioCmd.test | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/ioCmd.test b/tests/ioCmd.test index f021ade..184f773 100644 --- a/tests/ioCmd.test +++ b/tests/ioCmd.test @@ -774,6 +774,22 @@ test iocmd-21.20 {Bug 88aef05cda} -setup { close $ch rename foo {} } -match glob -result {1 {*nested eval*}} +test iocmd-21.21 {[close] in [read] segfaults} -setup { + proc foo {method chan args} { + switch -- $method initialize { + return {initialize finalize watch read} + } finalize {} watch {} read { + close $chan + return a + } + } + set ch [chan create read foo] +} -body { + read $ch 1 +} -cleanup { + close $ch + rename foo {} +} -result a # --- --- --- --------- --------- --------- # Helper commands to record the arguments to handler methods. -- cgit v0.12 From a07adf4d6d28771d9aa74ef06526e5fb3035f5c1 Mon Sep 17 00:00:00 2001 From: dgp Date: Mon, 21 Apr 2014 18:55:41 +0000 Subject: Added a refcounting mechanism to ChannelBuffers. Other edits to stop segfaults in tests iocmd-21.2[12]. --- generic/tclIO.c | 50 ++++++++++++++++++++++++++++++++++++++++++-------- generic/tclIO.h | 1 + generic/tclIOCmd.c | 3 +++ tests/ioCmd.test | 20 ++++++++++++++++++-- 4 files changed, 64 insertions(+), 10 deletions(-) diff --git a/generic/tclIO.c b/generic/tclIO.c index 9e675c6..a60c97d 100644 --- a/generic/tclIO.c +++ b/generic/tclIO.c @@ -162,6 +162,8 @@ typedef struct CloseCallback { */ static ChannelBuffer * AllocChannelBuffer(int length); +static void PreserveChannelBuffer(ChannelBuffer *bufPtr); +static void ReleaseChannelBuffer(ChannelBuffer *bufPtr); static void ChannelTimerProc(ClientData clientData); static int CheckChannelErrors(ChannelState *statePtr, int direction); @@ -352,6 +354,7 @@ ChanRead( int *errnoPtr) { if (WillRead(chanPtr) < 0) { + *errnoPtr = Tcl_GetErrno(); return -1; } @@ -2216,8 +2219,26 @@ AllocChannelBuffer( bufPtr->nextRemoved = BUFFER_PADDING; bufPtr->bufLength = length + BUFFER_PADDING; bufPtr->nextPtr = NULL; + bufPtr->refCount = 1; return bufPtr; } + +static void +PreserveChannelBuffer( + ChannelBuffer *bufPtr) +{ + bufPtr->refCount++; +} + +static void +ReleaseChannelBuffer( + ChannelBuffer *bufPtr) +{ + if (--bufPtr->refCount) { + return; + } + ckfree((char *) bufPtr); +} /* *---------------------------------------------------------------------- @@ -2250,7 +2271,7 @@ RecycleBuffer( */ if (mustDiscard) { - ckfree((char *) bufPtr); + ReleaseChannelBuffer(bufPtr); return; } @@ -2261,7 +2282,7 @@ RecycleBuffer( */ if ((bufPtr->bufLength - BUFFER_PADDING) < statePtr->bufSize) { - ckfree((char *) bufPtr); + ReleaseChannelBuffer(bufPtr); return; } @@ -2296,7 +2317,7 @@ RecycleBuffer( * If we reached this code we return the buffer to the OS. */ - ckfree((char *) bufPtr); + ReleaseChannelBuffer(bufPtr); return; keepBuffer: @@ -2470,6 +2491,7 @@ FlushChannel( * Produce the output on the channel. */ + PreserveChannelBuffer(bufPtr); toWrite = BytesLeft(bufPtr); if (toWrite == 0) { written = 0; @@ -2597,6 +2619,7 @@ FlushChannel( } RecycleBuffer(statePtr, bufPtr, 0); } + ReleaseChannelBuffer(bufPtr); } /* Closes "while (1)". */ /* @@ -2681,7 +2704,7 @@ CloseChannel( */ if (statePtr->curOutPtr != NULL) { - ckfree((char *) statePtr->curOutPtr); + ReleaseChannelBuffer(statePtr->curOutPtr); statePtr->curOutPtr = NULL; } @@ -3566,6 +3589,11 @@ static void WillWrite(Channel *chanPtr) static int WillRead(Channel *chanPtr) { + if (chanPtr->typePtr == NULL) { + /* Prevent read attempts on a closed channel */ + Tcl_SetErrno(EINVAL); + return -1; + } if ((chanPtr->typePtr->seekProc != NULL) && (Tcl_OutputBuffered((Tcl_Channel) chanPtr) > 0)) { if ((chanPtr->state->curOutPtr != NULL) @@ -3652,6 +3680,7 @@ Write( bufPtr->nextAdded += saved; saved = 0; } + PreserveChannelBuffer(bufPtr); dst = InsertPoint(bufPtr); dstLen = SpaceLeft(bufPtr); @@ -3665,6 +3694,7 @@ Write( if ((result != TCL_OK) && (srcRead + dstWrote == 0)) { /* We're reading from invalid/incomplete UTF-8 */ + ReleaseChannelBuffer(bufPtr); if (total == 0) { Tcl_SetErrno(EINVAL); return -1; @@ -3748,6 +3778,7 @@ Write( needNlFlush = 0; } } + ReleaseChannelBuffer(bufPtr); } if ((flushed < total) && (statePtr->flags & CHANNEL_UNBUFFERED || (needNlFlush && statePtr->flags & CHANNEL_LINEBUFFERED))) { @@ -5935,7 +5966,7 @@ DiscardInputQueued( */ if (discardSavedBuffers && statePtr->saveInBufPtr != NULL) { - ckfree((char *) statePtr->saveInBufPtr); + ReleaseChannelBuffer(statePtr->saveInBufPtr); statePtr->saveInBufPtr = NULL; } } @@ -6028,7 +6059,7 @@ GetInput( if ((bufPtr != NULL) && (bufPtr->bufLength - BUFFER_PADDING < statePtr->bufSize)) { - ckfree((char *) bufPtr); + ReleaseChannelBuffer(bufPtr); bufPtr = NULL; } @@ -6090,6 +6121,7 @@ GetInput( } else { #endif /* TCL_IO_TRACK_OS_FOR_DRIVER_WITH_BAD_BLOCKING */ + PreserveChannelBuffer(bufPtr); nread = ChanRead(chanPtr, InsertPoint(bufPtr), toRead, &result); #ifdef TCL_IO_TRACK_OS_FOR_DRIVER_WITH_BAD_BLOCKING @@ -6097,6 +6129,7 @@ GetInput( #endif /* TCL_IO_TRACK_OS_FOR_DRIVER_WITH_BAD_BLOCKING */ if (nread > 0) { + result = 0; bufPtr->nextAdded += nread; /* @@ -6122,6 +6155,7 @@ GetInput( #endif /* TCL_IO_TRACK_OS_FOR_DRIVER_WITH_BAD_BLOCKING */ } else if (nread == 0) { + result = 0; SetFlag(statePtr, CHANNEL_EOF); statePtr->inputEncodingFlags |= TCL_ENCODING_END; } else if (nread < 0) { @@ -6130,9 +6164,9 @@ GetInput( result = EAGAIN; } Tcl_SetErrno(result); - return result; } - return 0; + ReleaseChannelBuffer(bufPtr); + return result; } /* diff --git a/generic/tclIO.h b/generic/tclIO.h index ebf2ef7..0ea7d1c 100644 --- a/generic/tclIO.h +++ b/generic/tclIO.h @@ -36,6 +36,7 @@ */ typedef struct ChannelBuffer { + int refCount; /* Current uses count */ int nextAdded; /* The next position into which a character * will be put in the buffer. */ int nextRemoved; /* Position of next byte to be removed from diff --git a/generic/tclIOCmd.c b/generic/tclIOCmd.c index 2958bc8..7e8e91a 100644 --- a/generic/tclIOCmd.c +++ b/generic/tclIOCmd.c @@ -447,6 +447,7 @@ Tcl_ReadObjCmd( resultPtr = Tcl_NewObj(); Tcl_IncrRefCount(resultPtr); + Tcl_Preserve(chan); charactersRead = Tcl_ReadChars(chan, resultPtr, toRead, 0); if (charactersRead < 0) { /* @@ -462,6 +463,7 @@ Tcl_ReadObjCmd( TclGetString(chanObjPtr), "\": ", Tcl_PosixError(interp), NULL); } + Tcl_Release(chan); Tcl_DecrRefCount(resultPtr); return TCL_ERROR; } @@ -480,6 +482,7 @@ Tcl_ReadObjCmd( } } Tcl_SetObjResult(interp, resultPtr); + Tcl_Release(chan); Tcl_DecrRefCount(resultPtr); return TCL_OK; } diff --git a/tests/ioCmd.test b/tests/ioCmd.test index 184f773..8bf32d2 100644 --- a/tests/ioCmd.test +++ b/tests/ioCmd.test @@ -785,11 +785,27 @@ test iocmd-21.21 {[close] in [read] segfaults} -setup { } set ch [chan create read foo] } -body { - read $ch 1 + read $ch 0 } -cleanup { close $ch rename foo {} -} -result a +} -result {} +test iocmd-21.22 {[close] in [read] segfaults} -setup { + proc foo {method chan args} { + switch -- $method initialize { + return {initialize finalize watch read} + } finalize {} watch {} read { + catch {close $chan} + return a + } + } + set ch [chan create read foo] +} -body { + read $ch 1 +} -returnCodes error -cleanup { + catch {close $ch} + rename foo {} +} -match glob -result {*invalid argument*} # --- --- --- --------- --------- --------- # Helper commands to record the arguments to handler methods. -- cgit v0.12