Tcl Source Code

All tests have passed too... So I'll merge it to mainline branches.

Not reviewed yet but there is still a leak - looks like a different one though, and again not related to your changes but exposed by them, I think.

Only the tail of the log is below as it is quite large. Let me know if it would help to have the full log.

repository:   d:\src\repos\tcl.fossil
local-root:   c:/src/tcltk/wip/tcl/
config-db:    C:/Users/apnad/AppData/Local/_fossil
checkout:     47cb98a686c4276bab91492c7a31265ae9be56d6 2024-05-16 20:28:04 UTC
...
==5186== 6,440 (48 direct, 6,392 indirect) bytes in 1 blocks are definitely lost in loss record 113 of 113
==5186==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==5186==    by 0x488CB64: TclpAlloc (tclAlloc.c:699)
==5186==    by 0x48A7273: Tcl_Alloc (tclCkalloc.c:1059)
==5186==    by 0x49D7CEA: Tcl_DuplicateObj (tclObj.c:1587)
==5186==    by 0x49F4AE2: Tcl_GetReturnOptions (tclResult.c:1588)
==5186==    by 0x49B6DC9: MarshallError (tclIORChan.c:997)
==5186==    by 0x49B8E4B: InvokeTclMethod (tclIORChan.c:2380)
==5186==    by 0x49B7566: ReflectInput (tclIORChan.c:1331)
==5186==    by 0x49A3E66: ChanRead (tclIO.c:425)
==5186==    by 0x49ABE48: GetInput (tclIO.c:6941)
==5186==    by 0x49B09C8: DoRead (tclIO.c:9955)
==5186==    by 0x49AFE46: CopyData (tclIO.c:9627)
==5186==    by 0x49AF35E: TclCopyChannel (tclIO.c:9294)
==5186==    by 0x49B58BF: Tcl_FcopyObjCmd (tclIOCmd.c:1719)
==5186==    by 0x4899156: Dispatch (tclBasic.c:4503)
==5186==    by 0x48991E0: TclNRRunCallbacks (tclBasic.c:4539)
==5186==    by 0x489BECD: TclEvalObjEx (tclBasic.c:6104)
==5186==    by 0x489BE5D: Tcl_EvalObjEx (tclBasic.c:6085)
==5186==    by 0x49AECAD: TclChannelEventScriptInvoker (tclIO.c:8996)
==5186==    by 0x49AE27A: Tcl_NotifyChannel (tclIO.c:8455)
==5186==    by 0x4A24AE6: FileWatchNotifyChannelWrapper (tclUnixChan.c:491)
==5186==    by 0x4A3233E: FileHandlerEventProc (tclUnixNotfy.c:808)
==5186==    by 0x49D6B71: Tcl_ServiceEvent (tclNotify.c:670)
==5186==    by 0x49D6FC1: Tcl_DoOneEvent (tclNotify.c:967)
==5186==
==5186== LEAK SUMMARY:
==5186==    definitely lost: 48 bytes in 1 blocks
==5186==    indirectly lost: 6,392 bytes in 129 blocks
==5186==      possibly lost: 0 bytes in 0 blocks
==5186==    still reachable: 0 bytes in 0 blocks
==5186==         suppressed: 0 bytes in 0 blocks

OK, it looks like a name of reflected channel itself (the object is rcPtr->name, and after ref-decrement in FreeReflectedChannel its refCount is still 2, so has been not removed and hold the statePtr of channel within).

The issue is therefore not related to this ticket and/or fix...
It is simply a weird constellation: object containing the channel name (rc0) is referenced in reflected channel itself, and if the channel gets closed as long as its state is preserved several times (so FreeReflectedChannel doesn't remove it), a typical cyclic reference gets created:

• rcPtr->name (Tcl_Obj of channel type) holds the ref to statePtr,
• statePtr holds the ref to channel,
• chanPtr holds the ref to reflected (rcPtr via chanPtr->instanceData),
• rcPtr holds the ref to rcPtr->name (and cycle got closed).

I'll try to solve that somehow as part of this fix.

It looks like 1 object (03A09940) containing rc0 ($ch) doesn't get decremented, so still hold a reference to statePtr (and therefore it is not released yet), whereas another object (03A09B08), also containing rc0, is removed.

Here is a debug excerpt explaining that (3x preserve / 2x release of statePtr 03D933F0):

  ***==st allocat: 03D933F0 / Tcl_CreateChannel
  ***++st preserv: 03D933F0 / TclGetChannelFromObj === 03A09B08: rc0 ==
  ***++st preserv: 03D933F0 / TclGetChannelFromObj === 03A09940: rc0 ==
  ***++st preserv: 03D933F0 / Tcl_UnregisterChannel
  ***--st release: 03D933F0 / Tcl_UnregisterChannel
  ***--st release: 03D933F0 / FreeChannelInternalRep === 03A09B08

Maybe it is something like ScriptRecord (script of chan event as list containing $ch as apply parameter) or some interim duplication (which then becomes a leak). However related the whole profiler output, all CreateScriptRecord are paired by DeleteScriptRecord.

Hmm, indeed... I see 3 times Tcl_Preserve and only 2 times Tcl_Release for first statePtr (would correspond chan create from test)...
(but there are definitely others - e. g. 2 times Tcl_Preserve and only 1 time Tcl_Release for the second statePtr).

I'm also unsure it'd be caused by the fix directly, probably just introduced by possibility to do such things now. Investigating deeper.

Sorry, forgot to include the output

make -s valgrind TESTFLAGS="-f ioCmd.test -match iocmd-32.3"
...
==52905== 6,728 (264 direct, 6,464 indirect) bytes in 1 blocks are definitely lost in loss record 115 of 115
==52905==    at 0x484280F: malloc (vg_replace_malloc.c:442)
==52905==    by 0x4894FA5: TclpAlloc (tclAlloc.c:699)
==52905==    by 0x48AE1C9: Tcl_Alloc (tclCkalloc.c:1059)
==52905==    by 0x49A2745: Tcl_CreateChannel (tclIO.c:1630)
==52905==    by 0x49B332E: TclChanCreateObjCmd (tclIORChan.c:665)
==52905==    by 0x48A0C55: Dispatch (tclBasic.c:4503)
==52905==    by 0x48A0CDB: TclNRRunCallbacks (tclBasic.c:4539)
==52905==    by 0x48A0594: Tcl_EvalObjv (tclBasic.c:4262)
==52905==    by 0x48A2A7D: TclEvalEx (tclBasic.c:5408)
==52905==    by 0x49BE690: Tcl_FSEvalFileEx (tclIOUtil.c:1821)
==52905==    by 0x49C99AB: Tcl_MainEx (tclMain.c:403)
==52905==    by 0x4012F4: main (tclAppInit.c:91)
==52905==
==52905== LEAK SUMMARY:
==52905==    definitely lost: 264 bytes in 1 blocks
==52905==    indirectly lost: 6,464 bytes in 131 blocks
==52905==      possibly lost: 0 bytes in 0 blocks
==52905==    still reachable: 0 bytes in 0 blocks
==52905==         suppressed: 0 bytes in 0 blocks
==52905==

Looks good to merge to me.

All CI flows have passed... the latest changes are irrelevant for that, so I cancelled the ci-tag for now and set ticket to pending/fixed (and assigned to Ashok).

Here you go - in branch fix-79474c58800cdf94 you'd find a PR with a test and fix.

And you was right, solely protection by ref-counting of csPtr doesn't always work (additionally it needs preserving of referenced r/w channels as well as small change in DoRead exiting with an error if no bufPtr is there). Anyway it works as expected now (and I also did not see any leak there after implementation of ref-counter).

I'll merge it later to 8.6 if no objections follow.

Ref counting csPtr was the very first thing that had come to my mind. Then I saw dgp had already attempted a fix on similar basis and then backed it out. See his comments in https://core.tcl-lang.org/tcl/info/32ae34e63a and the code modifications in https://github.com/tcltk/tcl/commit/1f6b04d51b1c1270bf18e1535567f54c952a3158

Perusing that code led me to realize the ref count based fixes, which were on very similar lines, were not as simple as I had initially imagined. That is where I concluded fixing for 9.0 would be a daunting task given the time frame.

It would be great if you have a solution.

Unfortunately, the issues that caused dgp to back out his fix do not seem to be listed anywhere.

For the record, this bug is still present in Tcl 9.0b2.

Looking at the core dump, the root cause seems similar to that in 88aef05cd - reentrancy into TclInvokeMethod. A cursory inspection of the code shows the refchan read method in the test calling close which nests a call into finalize releasing various resources. As the stack unwinds at the top level TclInvokeMethod, freed memory is accessed resulting in the crash.

Fixing would require either

1. Protection of all data structures to handle reentrant calls

2. Forbid any recursive calls into refchans from the script level refchan as suggested by @aku in the above mentioned ticket as one of the options.

The former feels a daunting task for 9.0, though I'm not sure exactly how much work is required. The latter is likely simpler (though not trivial) but would mean backward incompatibility (implying it's either done for 9.0 or have to wait till 10.0), loss of function, and would require a TIP.