Re: [PATCH v2] conf: Unify several paths in conf_ports()

[Paul Holzinger <pholzing@redhat.com>]

On 15/03/2025 00:50, Stefano Brivio wrote:
> On Wed, 12 Mar 2025 14:43:59 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
>> In conf_ports() we have three different paths which actually do the setup
>> of an individual forwarded port: one for the "all" case, one for the
>> exclusions only case and one for the range of ports with possible
>> exclusions case.
>>
>> We can unify those cases using a new helper which handles a single range
>> of ports, with a bitmap of exclusions.  Although this is slightly longer
>> (largely due to the new helpers function comment), it reduces duplicated
>> logic.  It will also make future improvements to the tracking of port
>> forwards easier.
>>
>> The new conf_ports_range_except() function has a pretty prodigious
>> parameter list, but I still think it's an overall improvement in conceptual
>> complexity.
>>
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>>   conf.c | 173 ++++++++++++++++++++++++++++++---------------------------
>>   1 file changed, 90 insertions(+), 83 deletions(-)
>>
>> v2:
>>   * Commit message updated slightly, but otherwise unmodified.
>>
>>
>> diff --git a/conf.c b/conf.c
>> index 065e7201..4e0099ba 100644
>> --- a/conf.c
>> +++ b/conf.c
>> @@ -123,6 +123,75 @@ static int parse_port_range(const char *s, char **endptr,
>>   	return 0;
>>   }
>>   
>> +/**
>> + * conf_ports_range_except() - Set up forwarding for a range of ports minus a
>> + *                             bitmap of exclusions
>> + * @c:		Execution context
>> + * @optname:	Short option name, t, T, u, or U
>> + * @optarg:	Option argument (port specification)
>> + * @fwd:	Pointer to @fwd_ports to be updated
>> + * @addr:	Listening address
>> + * @ifname:	Listening interface
>> + * @first:	First port to forward
>> + * @last:	Last port to forward
>> + * @exclude:	Bitmap of ports to exclude
>> + * @to:		Port to translate @first to when forwarding
>> + * @weak:	Ignore errors, as long as at least one port is mapped
>> + */
>> +static void conf_ports_range_except(const struct ctx *c, char optname,
>> +				    const char *optarg, struct fwd_ports *fwd,
>> +				    const union inany_addr *addr,
>> +				    const char *ifname,
>> +				    uint16_t first, uint16_t last,
>> +				    const uint8_t *exclude, uint16_t to,
>> +				    bool weak)
>> +{
>> +	bool bound_one = false;
>> +	unsigned i;
>> +	int ret;
>> +
>> +	if (first == 0) {
>> +		die("Can't forward port 0 for option '-%c %s'",
>> +		    optname, optarg);
>> +	}
> This introduces two subtle functional changes that are a bit unexpected
> given the commit message. Before:
>
> $ ./pasta -t 0
> $
>
> $ ./pasta -t 0-1025
> Failed to bind port 1 (Permission denied) for option '-t 0-1025', exiting
>
> After:
>
> $ ./pasta -t 0
> Can't forward port 0 for option '-t 0'
>
> $ ./pasta -t 0-1025
> Can't forward port 0 for option '-t 0-1025'
>
> ...anyway, I doubt anybody would use -t 0 on purpose (to get a port
> automatically assigned), and while it probably works for TCP (check
> bound ports after starting pasta, use the assigned one), it wouldn't
> necessarily work as expected for UDP if the application relies on our
> flow tracking.
Why would this not work for UDP? bind() wise you can still bind 0 fine 
and get a free port assigned?
>
> For TCP, actually, -t 0 might be useful, see e.g. random_free_port() in
> Podman tests (/test/system/helpers.network.bash). We should print the
> port number that was bound, though, and document the feature.
>
> More than that: that could actually be the only race-free possibility
> of picking and forwarding a port where the number doesn't matter.

Yes it could be useful for podman but then it should also work with udp. 
I am less worried about the tests, this issue is in podman proper as you 
can do "-p 80", then podman assigns a free host port. Except that this 
is super broken in podman because we do this once when we create the 
container so this is totally racy and non conflict free[1]. The thing of 
course is for podman we have to deal with like 4 other port forwarder 
implementations that we would need to support. As such I don't see us 
ever finding time to properly fix it unless it magically gets a ton of 
priority. So if pasta does not support for it I have no problems with 
that, however maybe one day we like to reconsider.

[1] 
https://github.com/containers/podman/issues/10205#issuecomment-1010055023

>
> In any case, given that it works by mistake now and it's undocumented,
> let me go ahead and apply this for the moment. We can add the
> "functionality" back later if it makes sense.
>
-- 
Paul Holzinger