Project

General

Profile

Bug #4602

memory leak in ILB daemon on import-config

Added by Serghei Samsi over 6 years ago. Updated over 6 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
networking
Start date:
2014-02-15
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:

Description

Starting ILBD (L3/L4 Integrated Load Balancer daemon) leads to memory leak:

umem_alloc_160 leak: 1 buffer, 160 bytes
ADDR BUFADDR TIMESTAMP THREAD
CACHE LASTLOG CONTENTS
8677670 8676f28 21bc28a68a0604 1
8630010 0 0
libumem.so.1`umem_cache_alloc_debug+0x157
libumem.so.1`umem_cache_alloc+0x157
libumem.so.1`umem_alloc+0xd0
libumem.so.1`malloc+0x2d
libumem.so.1`realloc+0x4f
adjust_srv_info_cmd+0x31
i_update_ksrv_rules+0x57
ilbd_create_rule+0x16b
ilbd_scf_instance_walk_pg+0x18f
ilbd_walk_rule_pgs+0x3d
i_ilbd_read_config+0x54
main_loop+0x59
main+0x197
_start+0x7d

How to reproduce:
1. Run /usr/lib/inet/ilbd with LD_PRELOAD=/usr/lib/libumem.so

2. Run ilbadm import-config <path_to_ilb_config_file>

3. Run ilbadm import-config <path_to_ilb_config_file>

4. Run gcore on ilbd PID, MDB findleaks output:
CACHE LEAKED BUFCTL CALLER
08630010 1 086776e8 adjust_srv_info_cmd+0x31
08630010 6 08677760 adjust_srv_info_cmd+0x31
08630010 1 08677670 adjust_srv_info_cmd+0x31

History

#1

Updated by Serghei Samsi over 6 years ago

Fix for adjust_srv_info_cmd leak
(file /illumos-gate/usr/src/cmd/cmd-inet/usr.lib/ilbd/ilbd_rules.c):

/*
 * this function adds all servers in srvlist to the kernel(!) rule
 * the name of which is passed as argument.
 */
static ilb_status_t
i_update_ksrv_rules(char *name, ilbd_sg_t *sg, ilbd_rule_t *rl)
{
    ilb_status_t        rc;
    ilbd_srv_t        *srvp;
    ilb_servers_info_cmd_t    *kcmd = NULL;
    int            i;

    /*
     * If the servergroup doesn't have any servers associated with
     * it yet, there's nothing more to do here.
     */
    if (sg->isg_srvcount == 0)
        return (ILB_STATUS_OK);

    /*
     * walk the list of servers attached to this SG
     */
    srvp = list_head(&sg->isg_srvlist);
    for (i = 0; srvp != NULL; srvp = list_next(&sg->isg_srvlist, srvp)) {
        rc = adjust_srv_info_cmd(&kcmd, i);
        if (rc != ILB_STATUS_OK)
-            return (rc);
+        {
+            if (kcmd != NULL) free(kcmd);
+                return (rc);
+        }

        ILB_SGSRV_2_KSRV(&srvp->isv_srv, &kcmd->servers[i]);
        /*
         * "no port" means "copy rule's port" (for kernel rule)
         */
        if (kcmd->servers[i].min_port == 0) {
            kcmd->servers[i].min_port = rl->irl_minport;
            kcmd->servers[i].max_port = rl->irl_maxport;
        }
        i++;
    }

    kcmd->cmd = ILB_ADD_SERVERS;
    kcmd->num_servers = i;
    (void) strlcpy(kcmd->name, name, sizeof (kcmd->name));

    rc = do_ioctl(kcmd, 0);
    if (rc != ILB_STATUS_OK)
-        return (rc);
+    {
+        if (kcmd != NULL) free(kcmd);
+        return (rc);
+    }

    for (i = 0; i < kcmd->num_servers; i++) {
        int e;

        if ((e = kcmd->servers[i].err) != 0) {
            logerr("i_update_ksrv_rules " 
                "ioctl indicates failure: %s", strerror(e));
            rc = ilb_map_errno2ilbstat(e);
            /*
             * if adding even a single server failed, we need to
             * roll back the whole wad. We ignore any errors and
             * return the one that was returned by the first ioctl.
             */
            kcmd->cmd = ILB_DEL_SERVERS;
            (void) do_ioctl(kcmd, 0);
+            if (kcmd != NULL) free(kcmd);
            return (rc);
        }
    }

+    if (kcmd != NULL) free(kcmd);
    return (ILB_STATUS_OK);
}
#2

Updated by David Höppner over 6 years ago

You should run cstyle. Also maybe just use a goto label ("rollback_kcmd") like the rest of the code (don't repeat the "free".) Thanks.

#3

Updated by Serghei Samsi over 6 years ago

Thanks David. I have posted modified version.

Fix for adjust_srv_info_cmd leak
(file /illumos-gate/usr/src/cmd/cmd-inet/usr.lib/ilbd/ilbd_rules.c):

/*
 * this function adds all servers in srvlist to the kernel(!) rule
 * the name of which is passed as argument.
 */
static ilb_status_t
i_update_ksrv_rules(char *name, ilbd_sg_t *sg, ilbd_rule_t *rl)
{
-    ilb_status_t        rc;
+    ilb_status_t        rc = ILB_STATUS_OK;
    ilbd_srv_t        *srvp;
    ilb_servers_info_cmd_t    *kcmd = NULL;
    int            i;

    /*
     * If the servergroup doesn't have any servers associated with
     * it yet, there's nothing more to do here.
     */
    if (sg->isg_srvcount == 0)
-        return (ILB_STATUS_OK);
+        goto out;

    /*
     * walk the list of servers attached to this SG
     */
    srvp = list_head(&sg->isg_srvlist);
    for (i = 0; srvp != NULL; srvp = list_next(&sg->isg_srvlist, srvp)) {
        rc = adjust_srv_info_cmd(&kcmd, i);
        if (rc != ILB_STATUS_OK)
-            return (rc);
+            goto rollback_kcmd;

        ILB_SGSRV_2_KSRV(&srvp->isv_srv, &kcmd->servers[i]);
        /*
         * "no port" means "copy rule's port" (for kernel rule)
         */
        if (kcmd->servers[i].min_port == 0) {
            kcmd->servers[i].min_port = rl->irl_minport;
            kcmd->servers[i].max_port = rl->irl_maxport;
        }
        i++;
    }

    kcmd->cmd = ILB_ADD_SERVERS;
    kcmd->num_servers = i;
    (void) strlcpy(kcmd->name, name, sizeof (kcmd->name));

    rc = do_ioctl(kcmd, 0);
    if (rc != ILB_STATUS_OK)
-        return (rc);
+        goto rollback_kcmd;

    for (i = 0; i < kcmd->num_servers; i++) {
        int e;

        if ((e = kcmd->servers[i].err) != 0) {
            logerr("i_update_ksrv_rules " 
                "ioctl indicates failure: %s", strerror(e));
            rc = ilb_map_errno2ilbstat(e);
            /*
             * if adding even a single server failed, we need to
             * roll back the whole wad. We ignore any errors and
             * return the one that was returned by the first ioctl.
             */
            kcmd->cmd = ILB_DEL_SERVERS;
            (void) do_ioctl(kcmd, 0);
-            return (rc);
+            goto rollback_kcmd;
        }
    }

-    return (ILB_STATUS_OK);
+rollback_kcmd:
+    if (kcmd != NULL) { 
+        free(kcmd);
+        kcmd = NULL;
+    }
+out:
+    return (rc);
}
#5

Updated by Robert Mustacchi over 6 years ago

  • Status changed from New to Resolved
  • % Done changed from 10 to 100
  • Tags deleted (needs-triage)
#6

Updated by Electric Monk over 6 years ago

git commit 56b8f71e3a910fbd2820f6841b40bfd85f9673c2

Author: Serghei Samsi <sscdvp@gmail.com>

4601 memory leak in ILB daemon on startup
4602 memory leak in ILB daemon on import-config
4668 Memory leak in ilbd' new_req: getpeerucred() allocation isn't released at all
Reviewed by: Dan McDonald <danmcd@omniti.com>
Reviewed by: Marcel Telka <marcel.telka@nexenta.com>
Approved by: Robert Mustacchi <rm@joyent.com>

Also available in: Atom PDF