* [PATCH v2] net/tap-solaris: Fix resource leaks on error paths
2026-09-05 7:04 [PATCH v2 0/1] net/tap-solaris: Fix resource leaks on error paths Xiong Weimin
@ 2026-09-05 7:04 ` Xiong Weimin
2026-09-05 8:11 ` [PATCH v2 0/1] " Michael S. Tsirkin
1 sibling, 0 replies; 3+ messages in thread
From: Xiong Weimin @ 2026-09-05 7:04 UTC (permalink / raw)
To: mst; +Cc: jasowang, qemu-devel, Xiong Weimin
The tap_alloc() function has multiple error paths where opened file
descriptors (tap_fd, if_fd, ip_fd, arp_fd) and STREAMS multiplexors
(ip_muxid, arp_muxid) are not released before returning error. This
leads to resource leaks and, on subsequent calls to tap_alloc(),
to use-after-free because ip_fd is a function-static variable.
Fix this by introducing explicit cleanup labels and releasing all
resources on every error path, in reverse order of acquisition:
fail_ip_muxid: I_PUNLINK ip_muxid
fail_arp_fd: close(arp_fd)
fail_if_fd: close(if_fd)
fail_tap_fd: close(tap_fd); close(ip_fd) if it was opened
The SIOCSLIFMUXID failure path keeps its existing inline
I_PUNLINK arp_muxid / I_PUNLINK ip_muxid calls (those muxids were
both successfully established at that point) and then jumps
directly to fail_if_fd to skip the redundant muxid cleanup.
The ip_fd reset to 0 after close is intentional: ip_fd is a
function-static variable, so any subsequent tap_alloc() invocation
must not see a stale value, otherwise the `if (ip_fd) close(ip_fd);`
at the top of the function would close a fd that some other code
now owns.
Changes since v1:
- Replaced the mixed fall-through / explicit-goto label scheme
of v1 with a single descending goto chain at the bottom of
the function: each label only releases its own resource and
falls through to the next, matching the goto-cleanup pattern
used in net/tap-linux.c.
- The ip_fd close that v1 inserted right before SIOCSLIFMUXID
(which used a stale fd) has been removed. ip_fd is now only
closed in the failure paths.
- arp_muxid and ip_muxid are I_PUNLINK'd exactly once each.
v1's separate fail_arp_muxid label would have double-released
them when the SIOCSLIFMUXID failure path fell through to it.
- This v2 is generated with `git diff` against a fresh checkout
of net/tap-solaris.c, so the file mode and hunks are well-
formed and `git apply --check` passes cleanly. v1's file mode
line was corrupted.
This refactor has not been tested on a real Solaris host, because
I do not have access to one. Please flag anything that looks off.
Signed-off-by: Xiong Weimin <xiongweimin@kylinos.cn>
---
net/tap-solaris.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/net/tap-solaris.c b/net/tap-solaris.c
index 8704b10..7ad111f 100644
--- a/tap-solaris.c
+++ b/net/tap-solaris.c
@@ -149,15 +149,18 @@ static int tap_alloc(char *dev, size_t dev_size, Error **errp)
strioc_if.ic_dp = (char *)𝔦
if (ioctl(arp_fd, I_STR, &strioc_if) < 0){
error_report("Can't set ifname to arp");
+ goto fail_arp_fd;
}
if((ip_muxid = ioctl(ip_fd, I_LINK, if_fd)) < 0){
error_setg(errp, "Can't link TAP device to IP");
- return -1;
+ goto fail_arp_fd;
}
- if ((arp_muxid = ioctl (ip_fd, link_type, arp_fd)) < 0)
+ if ((arp_muxid = ioctl (ip_fd, link_type, arp_fd)) < 0) {
error_report("Can't link TAP device to ARP");
+ goto fail_ip_muxid;
+ }
close (if_fd);
@@ -171,10 +174,25 @@ static int tap_alloc(char *dev, size_t dev_size, Error **errp)
ioctl (ip_fd, I_PUNLINK , arp_muxid);
ioctl (ip_fd, I_PUNLINK, ip_muxid);
error_report("Can't set multiplexor id");
+ goto fail_if_fd;
}
snprintf(dev, dev_size, "tap%d", ppa);
return tap_fd;
+
+fail_ip_muxid:
+ ioctl(ip_fd, I_PUNLINK, ip_muxid);
+fail_arp_fd:
+ close(arp_fd);
+fail_if_fd:
+ close(if_fd);
+fail_tap_fd:
+ close(tap_fd);
+ if (ip_fd > 0) {
+ close(ip_fd);
+ ip_fd = 0;
+ }
+ return -1;
}
int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2 0/1] net/tap-solaris: Fix resource leaks on error paths
2026-09-05 7:04 [PATCH v2 0/1] net/tap-solaris: Fix resource leaks on error paths Xiong Weimin
2026-09-05 7:04 ` [PATCH v2] " Xiong Weimin
@ 2026-09-05 8:11 ` Michael S. Tsirkin
1 sibling, 0 replies; 3+ messages in thread
From: Michael S. Tsirkin @ 2026-09-05 8:11 UTC (permalink / raw)
To: Xiong Weimin; +Cc: jasowang, qemu-devel
On Sat, Sep 05, 2026 at 03:04:47PM +0800, Xiong Weimin wrote:
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Hi Michael,
>
> Thanks for the careful review of v1 -- you were right that v1 was
> not ready to merge. Let me walk through the issues and the fixes
> in this v2.
>
> 1. Patch format corruption
> -----------------------
> v1's diff ended up with "4321006" in the file mode line instead
> of "100644", so `git am` could not apply it. v2 is generated by
> `git diff` against a fresh checkout of net/tap-solaris.c from
> master; `git apply --check` and `patch -p1` both pass cleanly.
>
> 2. ip_fd was being closed while still in use
> -----------------------------------------
> In v1 I had `close(if_fd); close(ip_fd);` sitting next to each
> other right before the SIOCSLIFMUXID ioctl() block. But the
> next ioctl() uses ip_fd, so closing it there meant we were
> issuing ioctl() on a stale fd. v2 removes that close entirely:
> ip_fd is now closed only on the failure paths, and the SIOCSLIFMUXID
> block uses a fully valid ip_fd.
>
> 3. The label scheme was double-releasing muxids
> ---------------------------------------------
> v1 had separate `fail_arp_muxid` and `fail_arp_fd` labels, and
> the SIOCSLIFMUXID failure path did
> I_PUNLINK arp_muxid;
> I_PUNLINK ip_muxid;
> error_report(...);
> goto fail_arp_muxid;
> then fall-through into fail_arp_muxid which I_PUNLINK'd them
> *again*. v2 fixes this by keeping the inline I_PUNLINK pair
> in the SIOCSLIFMUXID failure path (those muxids were both
> successfully established at that point) and `goto fail_if_fd`
> to skip past the redundant muxid cleanup. The result is each
> muxid and each fd is closed exactly once on every error path.
>
> The new cleanup chain
> ---------------------
>
> fail_ip_muxid: I_PUNLINK ip_muxid
> fail_arp_fd: close(arp_fd)
> fail_if_fd: close(if_fd)
> fail_tap_fd: close(tap_fd); close(ip_fd) if it was opened; ip_fd = 0
>
> ip_fd is a function-static variable, so resetting it to 0 after
> close is required: the next tap_alloc() invocation starts with
> `if (ip_fd) close(ip_fd);`, and we must not close a fd that some
> other code now owns.
>
> I considered folding the SIOCSLIFMUXID failure path into the goto
> chain as well, so that *every* cleanup is centralised, but that
> would have to either (a) duplicate the I_PUNLINK pair across two
> labels, or (b) reorder the chain so muxid cleanup runs before
> SIOCSLIFMUXID -- which doesn't make sense because those muxids
> don't exist before SIOCSLIFMUXID validates them. I think the
> current "inline I_PUNLINK at SIOCSLIFMUXID failure + goto
> fail_if_fd" is the cleanest decomposition, but if you prefer
> fully-centralised cleanup I'm happy to refactor it that way.
>
> Testing
> -------
> I do not have access to a Solaris 11 host here, so this patch
> has not been tested on a real Solaris system. The change is
> purely a refactor of the existing control flow, and the goto-
> cleanup pattern mirrors the one used in net/tap-linux.c (which
> is tested regularly), so I am fairly confident -- but please
> flag anything that looks off and I will follow up.
>
> Could you please take another look?
Sorry, as long as it's not tested - not really interested.
> Thanks,
> Weimin
>
> ---
> Xiong Weimin (1):
> net/tap-solaris: Fix resource leaks on error paths
>
> net/tap-solaris.c | 25 +++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
^ permalink raw reply [flat|nested] 3+ messages in thread