public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
From: Stefano Brivio <sbrivio@redhat.com>
To: "Lawrence, Richard E" <rlawrence@tamu.edu>
Cc: "passt-dev@passt.top" <passt-dev@passt.top>
Subject: Re: [PATCH v3] pasta: Add --pass-fds option to skip closing file descriptors
Date: Wed, 04 Nov 2026 00:59:06 +0100 (CET)	[thread overview]
Message-ID: <20261104005905.67ee738c@elisabeth> (raw)
In-Reply-To: <DM3PR11MB871367EDC1EBB06E9F0F58BBDA6E2@DM3PR11MB8713.namprd11.prod.outlook.com>

On Thu, 29 Oct 2026 17:01:46 +0000
"Lawrence, Richard E" <rlawrence@tamu.edu> wrote:

> Howdy Stefano,
> 
> I had sent another message to the PATCH v2 thread, but it seems you
> missed it. Copying inline below (>>) so you don't have to go hunting
> for it.

Indeed, I missed it (even though I received it, I just skipped it
somehow, sorry).  Thanks for the summary.

> 
> ## Re: long option index.
> >> It turns out the minimum index for a long option is 2, not 1.
> >> conf() already correctly does it that way.  
> 
> (continued...) It's because long option index 1 is reserved for
> positional arguments or something like that (forgot the details).

Oh, oops, right (I thought just 0 was reserved, but that's not the
case).

> ## Re: relocating fd_tap conditionally
> >> I think I misunderstood your question about unconditionally
> >> relocating the fd_tap. Please clarify: are you suggesting that we
> >> should skip relocating fd_tap when we also skip closing leaked
> >> file descriptors? That design would not cause any of the bugs I
> >> imagined. For example, it would never affect passt mode because
> >> skipping is forbidden in passt mode anyways. If the user does tap
> >> | pasta --fd 0 --pass-fds cmd then the fd_tap stays on stdin, so
> >> the spawned command gets it on stdin; that is technically a
> >> feature ("--pass-fds preserves file descriptors"), but I still
> >> think it's very weird behavior.  
> 
> (continued...) But, given your more recent comments, I think I am
> still misunderstanding your question about unconditionally relocating
> fd_tap. So I need to ask a different question instead. Please
> clarify: is it ever OK to leave the fd_tap on 0? If we can leave
> fd_tap on 0 in pasta mode with pass-fds, then that greatly simpifies
> the logic.

I'm not sure, I instinctively don't see a big issue with that, but you
had valid reasons to suggest we should avoid it, so I thought we should
avoid it.

My suggestion on v3 didn't actually question that part.  I was
suggesting the condition to move fd_tap would be:

	/* Move the passed fd to a more convenient location (3):
	 * - on --pass-fds, only if it might conflict with standard streams
	 * - otherwise, unconditionally, to keep close_range() below simple
	 */
	if (c->fd_tap >= 0 && (!c->pass_fds || c->fd_tap < close_from)) {

meaning that it's never left on 0 anyway (close_from is 3).  That won't
work for the case below as you noted, though.

> ## Re: dup2 vs fcntl
> 
> Please allow me to clarify: the reason I switched from dup2 to fcntl
> is because I imagined a scenario where the user wants to both:
> 
>   1.
> pass the tap fd to pasta on 0, and also
>   2.
> pass something else through to bwrap on 3.

Oh, oops, I see.

> For example:
> 
> ```
> exec 3<<<"Hello from FD 3" && tap | pasta --fd 0 --pass-fds bwrap
> --bind-fd 3 /hello bash -c 'cat /hello |
> send-it-to-the-internet-or-something-idk' ``` In that scenario, using
> dup2 is a major problem. It closes the existing FD 3 and puts the tap
> there instead! So the user is now accidentally sending the tap to the
> internet instead of the data they intended to pass on 3.
> 
> One possible solution is to leave the tap on 0 instead.

Could some users perhaps actually want that?  Say:

  (something) | pasta --pass-fds -- pasta --pass-fds -- bwrap ...

But what you propose below still looks slightly preferable to me.

> (previously...)
> >> If you do insist that this weird behavior is in fact the correct
> >> behavior, then we can demote the fcntl call back to plain dup2
> >> with a comment that explains it is safe to close and reuse fd 3
> >> since the user did not request to pass-fds. This comment is
> >> necessary to prevent a future bug if someone refactors the code
> >> carelessly (dup2 closes fd 3 when user wants to pass fd 3). Please
> >> let me know if that change is still what you want.  
> 
> I call this the "unsafe" option. I can imagine that leaving the tap
> on 0 could cause bugs

It shouldn't, but avoiding that would be worthwhile because of point 5.
in:

  https://archives.passt.top/passt-dev/20260715183946.6121580a@elisabeth/

that is, if 0 is never a valid file descriptor, we can use it
(implicitly) for initialisations, which avoids the need for a fair
amount of code (and a bit of binary size as well).

> and I imagine that leaving dup2 in the code
> could re-introduce the clobbering bug at a later time.

I don't think it's such a big issue as the code dealing with this is
all in the same place and relatively easy to observe.

> ## New Proposal
> 
> I propose to always relocate the tap above 2, to avoid bugs, but the
> system call we use can be conditional:
> 
>   *
> dup2 when we're closing fds (because it's the easiest) and
>   *
> fcntl when we're leaving them open (because it's the safest)
> 
> that seems like the best of both worlds.

Yes, I tend to agree.  I have no idea what the resulting complexity
might be.  If it's too complicated, I'd say let's just allow 0 as
fd_tap... up to you.

The purpose of my comments to v3 was to ensure we don't make things
more complicated than they need to be, but I guess some additional
complexity is needed in any case after all.

-- 
Stefano


      reply	other threads:[~2026-11-03 23:59 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-26 17:03 Richard Lawrence
2026-10-28 22:38 ` Stefano Brivio
2026-10-29 17:01   ` Lawrence, Richard E
2026-11-03 23:59     ` Stefano Brivio [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20261104005905.67ee738c@elisabeth \
    --to=sbrivio@redhat.com \
    --cc=passt-dev@passt.top \
    --cc=rlawrence@tamu.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).