Tcl Source Code

I noticed that 'seek 0' on a file handle makes the handle accessible in all interps. I can't find any evidence in the docs that this is the intended behavior.

Example: The following script will dump 2 x four bytes from /dev/random:

interp create foo
interp create bar

set f [open /dev/random rb]

interp eval foo {
    proc bytes {fh} {
        binary scan [read $fh 4] H* hex
        puts $hex
        close $fh
    }
}

interp eval bar {
    proc bytes {fh} {
        binary scan [read $fh 4] H* hex
        puts $hex
        close $fh
    }
}

seek $f 0
interp eval foo [list bytes $f]
interp eval bar [list bytes $f]
close $f

Without the seek command it reports an error as expected:
can not find channel named "file5"
    while executing
"read $fh 4"
    (procedure "bytes" line 2)
    invoked from within
"bytes file5"
    invoked from within
"interp eval foo [list byte $f]"


SuSE linux 11.0
Kernel 2.6.25.18-0.2-pae
Tcl 8.5.5

Applied patches to both 8.5 and head.

I don't know enough of the internals of Tcl to be able to give a theoretical opinion about the patch. I have confirmed that it works in practice with the application where I found the issue.

One additional remark: I found that this bug was not present in Tcl 8.5.2. It must have been introduced in Tcl 8.5.3 or 8.5.4.

File Added - 304859: tcl86.diff-wu

File Added - 304858: tcl85.diff-wu

@dgp: A bit of clarification.

The functions of tclChannelType do indeed Tcl_Preserve/Release ChannelState pointers. This is however not because of my fix. They did that even before I put my hands into this particular pie. As such it is an integral part of this TclObjTyp I was not really willing to change for my fix. What I did was to extend this scheme to the Interp pointer I added to the intrep. It might actually not be necessary, as the Interp pointer is only copied and compared, never de-referenced, contrary to the ChannelState.

I should further note that ChannelState structures actually have a refCount. This however is used to count how many interpreters have the the channel Tcl_RegisterChannel()'ed in them, and a refCount > 0 prevents fully closing the channel. I do not believe that it is a good idea to use this refCount to count how many Tcl_Obj refer to this channel. As it means that a 'close $f' will never really close the channel, but only at some arbitrary time later when the last intrep goes away. Which actually may be never during the lifetime of an interp.

I will see that I can get a test done for 8.5 and 8.6 in the next days, and then check if may Preseve/Release of the Interp* was overkill. Likely yes. If so I will remove that part of the patch.

comments on the chat indicated the
solution here involved having the
intrep of a Tcl_Obj set a Tcl_Preserve
on a struct, with a matching Tcl_Release
when the intrep is shimmered away.

That's fine as a stopgap, but if at
all possible, the need to do that
means the preserved struct really
needs its own refCount field.

If interpreter boundaries are the only concern (the "preservation within an interp" issue is a great thing to have a test case on in any case) then the patch is good. Needs a test, but that can probably be adapted from the bug report.

I do not believe that an epoch is required.
SCFA checks the flags of the preserved channelState for CHANNEL_CLOSED.
If that is true it already invalidated and refetched the intrep.
This should handle the close/reopen case.

sounds like a reasonable fix. You also probably need to check an epoch counter or something like that so that if the channel is deleted and reallocated, that can be handled too...

Added patch containing my proposed fix.
dkf, schelte, please review.

File Added - 304737: channel-intrep.patch

Ok, I was possibly not too clear.
SetChannelFromAny (SCFA) uses Tcl_GetChannel.
However if and only if the intrep is not already tclChannelType. 
If the intrep is properly tclChannelType it pulls the ChannelState*
directly out of the intrep, and now we have direct access to a channel
which may not be known to the interpreter we are in, because the intrep
was set in a different interpreter.
So, even with a properly typed intrep we have to recheck that the channel
we got is actually known in the current interp, and the same.

Best would be to not only store channelstate*, but also interp* of the interp which set the intrep. The moment we are in a different interp we invalidate and recompute the intrep. sort of like cmd resolution intreps.

@jeffh: The intrep of tclChannelType is the statePtr (ChannelState) of a channel, without an association to an interp. This means a Tcl_Obj with vlaid intrep can carry a channel into an interp which actually doesn't know, and it will be properly used by direct access to its state, without looking it up anywhere.

Note 1: There is no Tcl_GetChannelFromObj(). Only a TclGetChannelFromObj(). I.e. internal, not public.
Note 2: Trying to trace where the fix is in all the unrelated changes is ... not fun. There do not seem to be any changes to fix it.

Tracing the TclGetChannelFromObj a bit it seems that in 8.6 something invalidates the intrep after the seek, before the 'puts' in interp foo is reached. So far no idea what.

Donal's changes do not look like they intentionally changed any behavior.  I'm skeptical that TclGetChannelFromObj is the source of the issue as it layers over Tcl_GetChannel, which maintains a table in the assocData of a single interp, and uses Tcl_Objs, which should not be shared.  Am I missing something?

Ok, coming to this I see the problem in 8.5, but not 8.6/Head. As I also saw a commit by dkf to tclIO.c I am assuming right now that the 'Refactoring tclIO.c' commit was (intended as) a fix for this. Meaning that I 'just' have to back port it. Is that correct, Donal ?

Diagnosis: leakage of file handle through Tcl_Obj internal rep. Problem is not [seek] itself, but rather Tcl_GetChannelFromObj.