Project

General

Profile

Bug #4668

Memory leak in ilbd' new_req: getpeerucred() allocation isn't released at all

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

Status:
Resolved
Priority:
Normal
Assignee:
Category:
networking
Start date:
2014-03-05
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:

Description

The fix for 6944407 (Possible memory leak in ilbd) doesn't take into account that getpeerucred() can allocate a piece of memory for ucred_t ** second arguments.
As per getpeerucred(3C) manual states:
"When successful, getpeerucred() stores the pointer to a
freshly allocated ucred_t in the memory location pointed to
by the ucred argument if that memory location contains the
null pointer. If the memory location is non-null, it will
reuse the existing ucred_t."

The leak could be reproduced easy by doing commands:

# ilbadm show-nat

or
# ilbadm import-config

E.g. when ilbd' new_req() is called internally.

Due to small amount of memory leaked (320 bytes per getpeerucred() call),
for some observable results one could run some hundreds of thousand times ilbadm command.

root@mirror1_gateway:~# echo "::findleaks -dvf" | mdb -p 18397
findleaks:                maximum buffers => 93
findleaks:                 actual buffers => 37
findleaks: 
findleaks:             potential pointers => 159488
findleaks:                     dismissals => 141950        (89.0%)
findleaks:                         misses => 14018         ( 8.7%)
findleaks:                           dups => 3483          ( 2.1%)
findleaks:                        follows => 37            ( 0.0%)
findleaks: 
findleaks:              peak memory usage => 57 kB
findleaks:               elapsed CPU time => 0.0 seconds
findleaks:              elapsed wall time => 0.0 seconds
findleaks: 
findleaks: no memory leaks detected
root@mirror1_gateway:~# 
root@mirror1_gateway:~# ilbadm import-config /root/
.bash_history                core.9526_1                  ilb_memleaks_mdb_output.txt  ilbd                         svc-ilbd_patch
.bashrc                      core.9526_2                  ilbadm.conf                  ilbd_patch                   
.profile                     ilb_idr1011/                 ilbadm.conf_1                svc-ilbd                     
root@mirror1_gateway:~# ilbadm import-config /root/ilbadm.conf
root@mirror1_gateway:~# echo "::findleaks -dvf" | mdb -p 18397
findleaks:                maximum buffers => 129
findleaks:                 actual buffers => 45
findleaks: 
findleaks:             potential pointers => 172800
findleaks:                     dismissals => 152358        (88.1%)
findleaks:                         misses => 16778         ( 9.7%)
findleaks:                           dups => 3625          ( 2.0%)
findleaks:                        follows => 39            ( 0.0%)
findleaks: 
findleaks:              peak memory usage => 58 kB
findleaks:               elapsed CPU time => 0.0 seconds
findleaks:              elapsed wall time => 0.0 seconds
findleaks: 
CACHE     LEAKED   BUFCTL CALLER
080c8010       6 0811b260 libc_hwcap1.so.1`_ucred_alloc+0x24
------------------------------------------------------------------------
   Total       6 buffers, 1920 bytes

umem_alloc_320 leak: 6 buffers, 320 bytes each, 1920 bytes total
            ADDR          BUFADDR        TIMESTAMP           THREAD
                            CACHE          LASTLOG         CONTENTS
         811b260          811edc0     a5ed5300e05d                1
                          80c8010          80ac578                0
                 libumem.so.1`umem_cache_alloc_debug+0x157
                 libumem.so.1`umem_cache_alloc+0x19d
                 libumem.so.1`umem_alloc+0xd0
                 libumem.so.1`malloc+0x2d
                 libc_hwcap1.so.1`_ucred_alloc+0x24
                 libc_hwcap1.so.1`getpeerucred+0x26
                 new_req+0xaa
                 main_loop+0xb1
                 main+0x197
                 _start+0x7d

root@mirror1_gateway:~# ilbadm import-config /root/ilbadm.conf
root@mirror1_gateway:~# 
root@mirror1_gateway:~# echo "::findleaks -dvf" | mdb -p 18397
findleaks:                maximum buffers => 135
findleaks:                 actual buffers => 51
findleaks: 
findleaks:             potential pointers => 172800
findleaks:                     dismissals => 152358        (88.1%)
findleaks:                         misses => 16778         ( 9.7%)
findleaks:                           dups => 3625          ( 2.0%)
findleaks:                        follows => 39            ( 0.0%)
findleaks: 
findleaks:              peak memory usage => 58 kB
findleaks:               elapsed CPU time => 0.0 seconds
findleaks:              elapsed wall time => 0.0 seconds
findleaks: 
CACHE     LEAKED   BUFCTL CALLER
080c8010      12 0811b260 libc_hwcap1.so.1`_ucred_alloc+0x24
------------------------------------------------------------------------
   Total      12 buffers, 3840 bytes

umem_alloc_320 leak: 12 buffers, 320 bytes each, 3840 bytes total
            ADDR          BUFADDR        TIMESTAMP           THREAD
                            CACHE          LASTLOG         CONTENTS
         811b260          811edc0     a5ed5300e05d                1
                          80c8010          80ac578                0
                 libumem.so.1`umem_cache_alloc_debug+0x157
                 libumem.so.1`umem_cache_alloc+0x19d
                 libumem.so.1`umem_alloc+0xd0
                 libumem.so.1`malloc+0x2d
                 libc_hwcap1.so.1`_ucred_alloc+0x24
                 libc_hwcap1.so.1`getpeerucred+0x26
                 new_req+0xaa
                 main_loop+0xb1
                 main+0x197
                 _start+0x7d

After some tests:

root@mirror1_gateway:~# echo "::findleaks -dvf" | mdb -p 18397
findleaks:                maximum buffers => 5087
findleaks:                 actual buffers => 5003
findleaks: 
findleaks:             potential pointers => 172800
findleaks:                     dismissals => 152346        (88.1%)
findleaks:                         misses => 16797         ( 9.7%)
findleaks:                           dups => 3618          ( 2.0%)
findleaks:                        follows => 39            ( 0.0%)
findleaks: 
findleaks:              peak memory usage => 484 kB
findleaks:               elapsed CPU time => 0.0 seconds
findleaks:              elapsed wall time => 0.2 seconds
findleaks: 
CACHE     LEAKED   BUFCTL CALLER
080c8010    4964 0811b260 libc_hwcap1.so.1`_ucred_alloc+0x24
------------------------------------------------------------------------
   Total    4964 buffers, 1588480 bytes

umem_alloc_320 leak: 4964 buffers, 320 bytes each, 1588480 bytes total
            ADDR          BUFADDR        TIMESTAMP           THREAD
                            CACHE          LASTLOG         CONTENTS
         811b260          811edc0     a5ed5300e05d                1
                          80c8010          80ac578                0
                 libumem.so.1`umem_cache_alloc_debug+0x157
                 libumem.so.1`umem_cache_alloc+0x19d
                 libumem.so.1`umem_alloc+0xd0
                 libumem.so.1`malloc+0x2d
                 libc_hwcap1.so.1`_ucred_alloc+0x24
                 libc_hwcap1.so.1`getpeerucred+0x26
                 new_req+0xaa
                 main_loop+0xb1
                 main+0x197
                 _start+0x7d

History

#1

Updated by Serghei Samsi over 5 years ago

Two fixes for memory leak in ilbd' new_req: getpeerucred()
(file /illumos-gate/usr/src/cmd/cmd-inet/usr.lib/ilbd/ilbd_main.c)

static void
ilbd_free_cli(ilbd_client_t *cli)
{
    (void) close(cli->cli_sd);
    if (cli->cli_cmd == ILBD_SHOW_NAT)
        ilbd_show_nat_cleanup();
    if (cli->cli_cmd == ILBD_SHOW_PERSIST)
        ilbd_show_sticky_cleanup();
    if (cli->cli_saved_reply != NULL)
        free(cli->cli_saved_reply);
+    if (cli->cli_peer_ucredp != NULL)
+        ucred_free(cli->cli_peer_ucredp);
    free(cli->cli_pw_buf);
    free(cli);
}

Parallel fixing some cstyle typos in new_req:

static void
new_req(int ev_port, int listener, void *ev_obj)
{
    struct sockaddr    sa;
    int        sa_len;
    int        new_sd;
    int        sflags;
-    ilbd_client_t    *cli;
+    ilbd_client_t    *cli = NULL;
    int        res;
    uid_t        uid;

    sa_len = sizeof (sa);
    if ((new_sd = accept(listener, &sa, &sa_len)) == -1) {
        /* don't log if we're out of file descriptors */
        if (errno != EINTR && errno != EMFILE)
            logperror("new_req: accept failed");
        goto done;
    }

    /* Set the new socket to be non-blocking. */
    if ((sflags = fcntl(new_sd, F_GETFL, 0)) == -1) {
        logperror("new_req: fcntl(F_GETFL)");
        goto clean_up;
    }
    if (fcntl(new_sd, F_SETFL, sflags | O_NONBLOCK) == -1) {
        logperror("new_req: fcntl(F_SETFL)");
        goto clean_up;
    }
    if (fcntl(new_sd, F_SETFD, FD_CLOEXEC) == -1) {
        logperror("new_req: fcntl(FD_CLOEXEC)");
        goto clean_up;
    }
    if ((cli = calloc(1, sizeof (ilbd_client_t))) == NULL) {
        logerr("new_req: malloc(ilbd_client_t)");
        goto clean_up;
    }
    res = getpeerucred(new_sd, &cli->cli_peer_ucredp);
    if (res == -1) {
        logperror("new_req: getpeerucred failed");
-        free(cli);
        goto clean_up;
    }
    if ((uid = ucred_getruid(cli->cli_peer_ucredp)) == (uid_t)-1) {
        logperror("new_req: ucred_getruid failed");
-        free(cli);
        goto clean_up;
    }
    cli->cli_pw_bufsz = (size_t)sysconf(_SC_GETPW_R_SIZE_MAX);
    if ((cli->cli_pw_buf = malloc(cli->cli_pw_bufsz)) == NULL) {
-        free(cli);
        logerr("new_req: malloc(cli_pw_buf)");
        goto clean_up;
    }
    if (getpwuid_r(uid, &cli->cli_pw, cli->cli_pw_buf,
        cli->cli_pw_bufsz) == NULL) {
-        free(cli->cli_pw_buf);
-        free(cli);
        logperror("new_req: invalid user");
        goto clean_up;
    }
    cli->cli_ev = ILBD_EVENT_REQ;
    cli->cli_sd = new_sd;
    cli->cli_cmd = ILBD_BAD_CMD;
    cli->cli_saved_reply = NULL;
    cli->cli_saved_size = 0;
    if (port_associate(ev_port, PORT_SOURCE_FD, new_sd, POLLRDNORM,
        cli) == -1) {
        logperror("new_req: port_associate(cli) failed");
-        free(cli->cli_pw_buf);
-        free(cli);
clean_up:
+        if (cli != NULL) {
+            if (cli->cli_pw_buf != NULL)
+                free(cli->cli_pw_buf);
+            if (cli->cli_peer_ucredp != NULL)
+                ucred_free(cli->cli_peer_ucredp);
+            free(cli);
+            cli = NULL;
+        }
        (void) close(new_sd);
    }

done:
    /* Re-associate the listener with the event port. */
    if (port_associate(ev_port, PORT_SOURCE_FD, listener, POLLRDNORM,
        ev_obj) == -1) {
        logperror("new_req: port_associate(listener) failed");
        exit(1);
    }
}

#3

Updated by Robert Mustacchi over 5 years ago

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

Updated by Electric Monk over 5 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