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

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Jul 24, 2026 at 05:38:09PM +0800, Weimin Xiong wrote:
> From: Xiong Weimin <xiongweimin@kylinos.cn>
> 
> The tap_alloc() function has multiple error paths where opened file
> descriptors (tap_fd, if_fd, ip_fd) are not closed before returning
> error. This leads to resource leaks.
> 
> Fix this by properly closing file descriptors before returning error
> in all error paths.
> 
> Signed-off-by: Xiong Weimin <xiongweimin@kylinos.cn>
> ---
>  net/tap-solaris.c | 47 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/net/tap-solaris.c b/net/tap-solaris.c
> index 1234567890ab..fedcba098765 4321006

mode 4321006? this line seems corrupt, at least it confuses my git am


> --- a/net/tap-solaris.c
> +++ b/net/tap-solaris.c
> @@ -60,8 +60,6 @@
>   */
>  static int tap_alloc(char *dev, size_t dev_size, Error **errp)
>  {
> -    /* FIXME leaks like a sieve on error paths */
> -    /* FIXME suspicious: many errors are reported, then ignored */
>      int tap_fd, if_fd, ppa = -1;
>      static int ip_fd = 0;
>      char *ptr;
> @@ -102,18 +100,30 @@ static int tap_alloc(char *dev, size_t dev_size, Error **errp)
>      strioc_ppa.ic_dp = (char *)&ppa;
>      if ((ppa = ioctl (tap_fd, I_STR, &strioc_ppa)) < 0) {
>          error_report("Can't assign new interface");
> -        return -1;
> +        goto fail_tap_fd;
>      }
>  
>      if_fd = RETRY_ON_EINTR(open("/dev/tap", O_RDWR, 0));
>      if (if_fd < 0) {
>          error_setg_file_open(errp, errno, "/dev/tap");
> -        return -1;
> +        goto fail_tap_fd;
>      }
>      if(ioctl(if_fd, I_PUSH, "ip") < 0){
>          error_setg(errp, "Can't push IP module");
> -        return -1;
> +        goto fail_if_fd;
> +    }
> +
> +    if (ioctl(if_fd, SIOCGLIFFLAGS, &ifr) < 0) {
> +        error_report("Can't get flags");
> +        goto fail_if_fd;
> +    }
> +
> +    snprintf(actual_name, 32, "tap%d", ppa);
> +    pstrcpy(ifr.lifr_name, sizeof(ifr.lifr_name), actual_name);
> +
> +    ifr.lifr_ppa = ppa;
> +    if (ioctl(if_fd, SIOCSLIFNAME, &ifr) < 0) {
> +        error_report("Can't set PPA %d", ppa);
> +        goto fail_if_fd;
>      }
>  
> -    if (ioctl(if_fd, SIOCGLIFFLAGS, &ifr) < 0)
> -        error_report("Can't get flags");
> +    if (ioctl(if_fd, SIOCGLIFFLAGS, &ifr) < 0) {
> +        error_report("Can't get flags");
> +        goto fail_if_fd;
> +    }
>  
> -    snprintf (actual_name, 32, "tap%d", ppa);
> -    pstrcpy(ifr.lifr_name, sizeof(ifr.lifr_name), actual_name);
> -
> -    ifr.lifr_ppa = ppa;
> -    /* Assign ppa according to the unit number returned by tun device */
> -
> -    if (ioctl (if_fd, SIOCSLIFNAME, &ifr) < 0)
> -        error_report("Can't set PPA %d", ppa);
> -    if (ioctl(if_fd, SIOCGLIFFLAGS, &ifr) <0)
> -        error_report("Can't get flags");
>      /* Push arp module to if_fd */
> -    if (ioctl (if_fd, I_PUSH, "arp") < 0)
> +    if (ioctl(if_fd, I_PUSH, "arp") < 0) {
>          error_report("Can't push ARP module (2)");
> +        goto fail_if_fd;
> +    }
>  
>      /* Push arp module to ip_fd */
> -    if (ioctl (ip_fd, I_POP, NULL) < 0)
> +    if (ioctl(ip_fd, I_POP, NULL) < 0) {
>          error_report("I_POP failed");
> -    if (ioctl (ip_fd, I_PUSH, "arp") < 0)
> +        goto fail_if_fd;
> +    }
> +    if (ioctl(ip_fd, I_PUSH, "arp") < 0) {
>          error_report("Can't push ARP module (3)");
> +        goto fail_if_fd;
> +    }
>      /* Open arp_fd */
>      arp_fd = RETRY_ON_EINTR(open("/dev/tap", O_RDWR, 0));
> -    if (arp_fd < 0)
> +    if (arp_fd < 0) {
>          error_report("Can't open %s", "/dev/tap");
> +        goto fail_if_fd;
> +    }
>  
>      /* Set ifname to arp */
>      strioc_if.ic_cmd = SIOCSLIFNAME;
> @@ -147,21 +172,37 @@ 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);
> +    close(ip_fd);


so you close ip_fd here ...

>  
>      memset(&ifr, 0x0, sizeof(ifr));
>      pstrcpy(ifr.lifr_name, sizeof(ifr.lifr_name), actual_name);
>      ifr.lifr_ip_muxid  = ip_muxid;
>      ifr.lifr_arp_muxid = arp_muxid;
>  
> -    if (ioctl (ip_fd, SIOCSLIFMUXID, &ifr) < 0)
> -    {
> -      ioctl (ip_fd, I_PUNLINK , arp_muxid);
> -      ioctl (ip_fd, I_PUNLINK, ip_muxid);
> -      error_report("Can't set multiplexor id");
> +    if (ioctl(ip_fd, SIOCSLIFMUXID, &ifr) < 0) {


and then try to do an ioctl on it?

> +        ioctl(ip_fd, I_PUNLINK, arp_muxid);
> +        ioctl(ip_fd, I_PUNLINK, ip_muxid);
> +        error_report("Can't set multiplexor id");
> +        goto fail_arp_muxid;
>      }
>  
>      snprintf(dev, dev_size, "tap%d", ppa);
>      return tap_fd;
> +
> +fail_arp_muxid:
> +    close(arp_fd);

arp_fd closed here

> +fail_ip_muxid:
> +    ioctl(ip_fd, I_PUNLINK, ip_muxid);
> +fail_arp_fd:
> +    close(arp_fd);

then again here

> +fail_if_fd:
> +    close(if_fd);
> +fail_tap_fd:
> +    close(tap_fd);
> +    if (ip_fd > 0) {
> +        close(ip_fd);
> +        ip_fd = 0;
> +    }
> +    return -1;
>  }