Repository navigation
Conversation
Author
|
cc @mvdan Can you please take a look |
aki-kong
force-pushed
the
feat/fifo-handler
branch
from
August 31, 2026 09:51
237224f to
354847a
Compare
Owner
|
Thanks. This PR is largely in the right direction but I'm afraid it is incorrect. You are removing part of a test which existed explicitly to cover #1120, and that bug is reintroduced here. I will take a look at a fix myself. |
Author
I felt the same. Somewhere there should be some sort of overlap/dependency between the pipe handler and the open handler. Let me know the approach, I can help with the changes as well (if you feel like it 😀) |
mvdan
added a commit
that referenced
this pull request
Aug 31, 2026
Process substitutions always created a real named pipe on disk via mkfifo, regardless of any handlers, breaking interpreters which sandbox all file I/O via a custom OpenHandler; see the discussion in #1120. The new handler owns the whole lifecycle of a process substitution: it names the path, opens the subshell's end, opens the path whenever the shell itself consumes it, and cleans up once the subshell has finished. The default handler keeps the old behavior using real named pipes. Since the runner now tracks its active process substitutions, Runner.open routes their paths to the handler directly, replacing the detection of FIFO paths by temporary directory and name prefix. An alternative to #1404. See #1120 for previous work in this space.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Process substitution always creates a real named pipe on disk via mkfifo
and os.OpenFile, no matter what OpenHandler/StatHandler/AccessHandler are
set to, so it's a hole straight to the real filesystem for anyone
sandboxing file I/O.
This is #1120, where you left the door open for a handler:
I've taken a crack at implementing it. ProcSubstOpen owns the whole FIFO
lifecycle (create, open, remove), since a plain (path, flag, perm) open
can't express mkfifo + cleanup. DefaultProcSubstOpen is the old logic
moved into handler.go, same behavior, except the actual open is now
deferred to the first read/write, since opening a FIFO blocks until a
peer connects and that peer isn't running yet when the path is returned.
This also meant removing the bit in r.open that special-cased FIFO
paths to skip OpenHandler, since creation is now handler-owned. Happy
to reconsider that if you'd rather keep it separate.
Updated the tests accordingly, including a new one (ProcSubstVirtual)
that pairs ProcSubstOpen with OpenHandler over an in-memory io.Pipe and
checks nothing touches the real filesystem.
Motivation
I'm using this shell intepreter package to implement a simple Bash sandbox with WASI as the execution provider which supports
fs.FS. I had implemented theOpenHandlerbased on thefs.FSbut just for the ProcessSubstitution there seemed to be a sandbox breakage. In this case, I can implement the ProcSub + OpenHandler to handle the Pipe implementation (plan to use channels). Left the DefaultHandler to the original behaviour.