[PATCH v2 0/1] net/tap-solaris: Fix resource leaks on error paths

[PATCH v2 0/1] net/tap-solaris: Fix resource leaks on error paths

[Xiong Weimin]

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?

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(-)

[PATCH v2] net/tap-solaris: Fix resource leaks on error paths

[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 *)&ifr;
     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,

Re: [PATCH v2 0/1] net/tap-solaris: Fix resource leaks on error paths

[Michael S. Tsirkin]

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(-)