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

Xiong Weimin posted 1 patch 3 weeks ago
Failed in applying to current master (apply log)
net/tap-solaris.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
[PATCH v2] net/tap-solaris: Fix resource leaks on error paths
Posted by Xiong Weimin 3 weeks ago
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,