Tcl Source Code

[source] sets -eofchar to the single-element list ^Z, which means it is interpreted both as the input and output EOF character.  When the channel is closed, the output EOF character is appended to the file.

This is normally not a problem because [source] opens files read-only, but when the file is actually a [tcl::chan::variable] reflected channel inside a custom VFS, the file is writable anyway, and the ^Z is written.

Work continuing in [https://core.tcl-lang.org/tips/doc/trunk/tip/646.md|TIP #646]

As long as we always use -eofchar "\032 {}", this doesn't cause any problem.

Nowadays, I don't see any reason to set the output part of -eofchar to something else as {}. And this ticket is about appending ˆZ, which is fixed now. For any other problem, better open a new ticket, if appropriate. So, closing.

I see multiple distinct issues which perhaps should be separate tickets, and not all in the Tcl core repository:

1. [source] appends ^Z.
2. [close] doesn't confirm it's truly at EOF before appending ^Z.
3. Several Tcllib refchans cannot be made read-only.
4. Several sample VFSes do not decode and pass along the mode.
5. The mode string passed by VFS to the open command is confusing.

1 is fixed in 8.6.7, but the investigation opened a can of worms.

2 probably could be fixed, though we'll need to discuss it more.

3 means changing the way the Tcllib refchans are instantiated.  For the sake of backward compatibility, giving them no additional mode arguments should make them be read-write.

4 is a direct consequence of 3, but I hypothesize that it in an alternate timeline in which 3 were never the case, 4 still could have been caused by 5.

Regarding 5: I'd like to see the mode string regularized such that VFS-to-open mode strings always be directly acceptable to refchans with no translation required.  How to do this compatibly?  Maybe an extra element being returned by initialize could signal the VFS which style to use.

Also regarding 5: The low-hanging fruit is to update the man page to say that read-only mode is denoted by empty string vice "r".

Regarding 4 and 5: Where is the VFS extension hosted and developed?

It's unfortunate that the EOF character "append" code assumes it's in fact sought (seeked?) to the end of file before it does its business.  I have a question about that.  Does it first check if the next character in the file is the EOF character, then elect not to write an EOF?  I think more likely it assumed that the rest of the I/O system would never let the file pointer pass the EOF, then writes the EOF character unconditionally, "knowing" it would either overwrite the existing EOF, or add one if it wasn't there already.  This theory is consistent with the behavior demonstrated by your "Pest" code.

Also, as you discovered, [seek] has nothing to do with triggering the "append EOF".  It's [close] that does that.  From the [chan configure -eofchar]/[fconfigure -eofchar] man page:  "For output, the end-of-file character is output when the channel is closed."  I think maybe the [close]/[chan close] man page should be updated to mention this as well.

Well this is certainly wrong:

% proc test {} {
    set fd [open test w+]
    chan configure $fd -eofchar P
    puts $fd "Test"
    seek $fd 0 start
    close $fd
}
% test
% exec cat test
Pest

CloseChannel() has written an eofchar wherever in the file it happens to be 
since at least 1998 - I didn't look further than [cacdd0f329].

I guess the question becomes:  when is it appropriate to append an "EOF 
character" to a file?  I suspect and hope the correct answer is "never".

Agreed on the last point - vfs should provide "r" in this instance, as 
documented.

One further thought:  the behaviour exhibited with [source] is happens on
regular fs/chans as well.  Is this really correct?

set fd [open test w]
puts $fd test
close $fd
puts [file size test]                   ;# "5"

set fd [open test r+]
chan configure $fd -eofchar "{} \x1a"
seek $fd 0 end
close $fd
puts [file size test]                   ;# "6"
file delete test

In the second part it has been opened rw, but simply seeking to the end is
enough to append eofchar.  Can that really be right?

Yes to all.  This is my understanding as well.

One point, which is a confusing one.  Despite what the VFS man page may say (at least the version I linked earlier, search for "http"), the read mode is empty string, not "r", coming from VFS to the user-supplied open command.  Oh, also there's no "r+".  In my opinion, "Properly decoding the mode argument" is much harder than it has to be.

Just to make sure I understand everything correctly:  to fix vfs and refchan 
implementations in the wild would require:

1:  [$vfs open] to properly decode the "mode" argument and pass it on to refchan.

2:  refchan implementations (such as ::vfs::memchan, ::tcl::chan::variable) to 
    take (require?) a mode argument and pass it on to [chan create]


If I'm observing correctly, [$vfs open] should also pay attention to whether 
truncation has been requested (w w+) and consider whether to return ENOENT
for (r r+).  It does not need to handle binary mode or seeking to the end.


I agree that this is a lot of detail for VFS implementations to get right (or 
risk subtle bugs) but I think we should try to get the reference implementations 
right - and well covered in tests - before plumbing magic channels.

> The custom VFS is responsible for translating the former to the latter, and it's so much easier to just say "read write" always

FWIW: Reflected channel can use custom configure-options, this can be used to transmit the mode to its handler...

Bellow is an excerpt example from my own fcgi-reflected channels:

<code><pre>
  proc initialize {ch args} {
    return {... configure ...}
  }
  proc configure {ch args} {
    foreach {n v} $args {
      switch -- $n \
      "-open-mode" {
        # ... change here the mode ...
        if {$v ni {w w a a+ ...}} { ... reset writable }
      } \
      "-add-header" {
        addHeader {*}$v
      } \
      "-end-request" {
        endRequest {*}$v
      } \
      ...
    }
  }
  ...
  # notice reflected channel handler the open mode:
  configure $ch -open-mode $mode
</pre></code>

Yes aku, you have it right, this is a confluence of issues, a problem that doesn't exist in any one subsystem but rather in the their integration.

The mode originates with the user script, either as an argument to [open] or implied "r" for [source].  It is sent to the VFS, which then sends it to the channel code.

We've gotten so flexible that several stages of this process are allowed to reflect back into the user script.  Both the VFS and the channel can be customized.  While I'm thrilled Tcl has grown in this direction, there's a lot that can go wrong.

We are trusting the user-supplied VFS to correctly relay the mode from the VFS subsystem to the I/O subsystem, which are two different realms.  The boundary is quite blurred since user VFSes often don't call [chan create] directly but instead use a wrapper supplied by the custom refchan package, yet despite this code being ostensibly part of the refchan, it is effectively part of the VFS.  Thus the VFS has responsibility without authority, which is always a bad combination.

When I say "two different realms" I have in mind the totally different ways VFS and refchan represent modes.  VFS has numeric (typically octal) permissions used in case a new file is created, but that has no bearing here except to sew confusion.  It also has a mode string which is either "", "w", "a", "w+", or "a+".  Contrast that with refchan which has a mode of "read", "write", or "read write".  The custom VFS is responsible for translating the former to the latter, and it's so much easier to just say "read write" always.  Wouldn't it make more sense to unify the representation of modes, to replace mode strings with refchan-style lists?

VFS modes say not only which operations are permitted, but also whether the initial file pointer should be at the end or the beginning.  This could be represented by having (or not having) "append" be in the mode list.  Then make refchan ignore "append" so the mode list can be passed along directly, eliminating any need for translation.  The goal is for the right thing to also be the easy thing so users are less likely to take shortcuts or make mistakes.

Further complicating the issue, the [open] command has two more similar but incompatible ways of describing modes.  It has mode strings, but they're not the same as VFS mode strings.  In particular, [open] supports "r" and "r+" which VFS encodes as "" and "a+".  It also supports a "b" qualifier.  In addition to mode strings, [open] allows lists of RDONLY, WRONLY, or RDWR plus any number of extra flags such as APPEND and BINARY.

One more issue to top it all off.  The VFS man page, which I found at http://jsish.org/man/mann/vfs.html, says the read-only open mode is "r" whereas in reality it's empty string.

So yeah, we can improve Tcllib to respect modes, but I think that just like my [source] change, that would also be missing the underlying issue, which is that it's much harder than it should be to correctly transmit the mode from the VFS subsystem to the I/O channel subsystem.  Until that's addressed, this bug will continue to crop up in user programs.

(Note: I am a bit distracted by having a relative visit and showing her around)

Would it make sense to extend the Tcllib refchans to take and obey mode arguments ? They were originally written as demonstrators. With them getting more used for production things their limitations as written become more apparent.

What about the memchan package and command. IIRC this does not take a mode either. And could.

With such a change the original (Tcl)VFS can be made good citizens too.

Do I read it right that we are running into a confluence of issues, where independent issues in core, tclvfs, tcllib refchans come together in a bug ?

The underlying problem is many VFS implementations call [chan create {read write}] without regard to the mode argument to [open].  The I/O channel subsystem is doing the right thing with the information it's being given.

What can be done?  As much as I'd like to say that we should fix the Tcllib VFSes to be good examples, this is impossible because they hide the [chan create] call within commands like [tcl::chan::memchan] and [tcl::chan::variable] which doesn't accept mode arguments anyway.

If we want to address this, I think we'd need a magic communication tunnel from the VFS [open] to the refchan [initialize] to avoid trusting the user code in the middle to pass the right mode to [chan create].  This seems fragile and distasteful to me, but what else can be done?

Maybe we say it's good enough that we fixed the one identified case of an inadvertent write happening on what was supposed to be a read-only channel, and just abandon the users to their fate should they explicitly set an output EOF character on a read-only channel that was secretly writable due to the custom VFS passing the wrong mode to [chan create].

I made another test program that cycles through many combinations of modes and operations, logging everything along the way.  Maybe it'll be helpful for studying the operation of VFS and refchan in general.

This needs more attention.  Many of the refchans in tcllib always have mode 0777 no matter what was passed to [open].  [close [open $file]] should never even try to write to the channel, even if an output EOF character was set, even if it was explicitly set by the user script.

On unpatched Tcl, I see the same behaviour if [source /test/foo] is replaced 
with:

set fd [open /test/foo r]
chan configure $fd -eofchar \032
read $fd
close $fd

::tcl::chan::variable always opens the channel {read write}, so of course it 
happens without vfs as well.  The call to write can be observed by a trace 
on the object constructed in the proc t:c:v.

I think this could be fixed in ::tcl::chan (probably in events?), but maybe 
it's worth the core protecting refchans that take this shortcut.

Attached script to demonstrate the problem.  Without check-in [527d354828], the script prints:

10
hello
11

Due to the ^Z being appended to $foo.  With the check-in, the script prints:

10
hello
10

I tested it with pure tcl reflected channel (without fix) and it seems to work also (but I don't know all constellations where it may be written nevertheless).

Thus I think Don has right and indeed it may be just a not sane implementation "inside a custom VFS" (write despite missing WRITABLE flag).

BTW the fix looks nevertheless good, IMHO.

I looked through the Tcl source code.

Tcl_FSEvalFileEx() opens the file with

Tcl_FSOpenFileChannel(interp, pathPtr, "r", 0644)

which should be a read-only open.

The various channel close pathways only append
the outEofChar to the channel if it exists and if
the channel is TCL_WRITABLE.

Unless there's something I miss, the only way
trouble can show up is if some Tcl_ChannelType
is coded wrong so that when it is open read only
it still sets the TCL_WRITABLE flag.

Is that Tcl_ChannelType in Tcl?

That said, I don't understand where this bug matters.

Why would [source] ever open its channel for writing?

And if it doesn't open for writing, why should we care
what that channel's output -eofchar is?

This smells like the real bug is somewhere else.

In the future please leave the *-rc branches to those
of us making releases.  I want to pull rather than get pushed to.

Merged to other branches that have lately been kept up-to-date, tried to replicate the style of recent merges, don't know if there is an official list of live branches deserving of being kept current.  Please confirm merges and close ticket if satisfied.

While merging, noticed win/tclWinChan.c, win/tclWinConsole.c, win/tclWinPipe.c, and win/tclWinSerial.c also set -eofchar but already correctly set the output EOF character to {}.  Thus no further change should be needed to ensure all platforms are handled.

+1, please backport tot other branches. Thanks!

Corrected with check-in [527d354828], please review.