#723 Possible memory leak in sss_nss_recv_rep
Closed: Fixed None Opened 13 years ago by sgallagh.

Calling allocation function "sss_nss_recv_rep" on "buf". [show details]
  319    ret = sss_nss_recv_rep(cmd, &buf, &len, errnop);
At conditional (1): "ret != 1" taking the true branch.
  320    if (ret != NSS_STATUS_SUCCESS) {
Variable "buf" going out of scope leaks the storage it points to.
  321        return ret;
  322    }

sss_nss_recv_rep() should be refactored so that len and buf is only assigned after the master while loop has completed.


Fields changed

milestone: NEEDS_TRIAGE => SSSD 1.5.0

Patch submitted: 5d6b7b9

However this revealed a second memory leak.

  172static enum nss_status sss_nss_recv_rep(enum sss_cli_command cmd,
  173                                        uint8_t **_buf, int *_len,
  174                                        int *errnop)
  175{
  176    uint32_t header[4];
  177    size_t datarecv;
  178    uint8_t *buf;
  179    int len;
  180
  181    header[0] = SSS_NSS_HEADER_SIZE; /* unitl we know the real lenght */
  182    header[1] = 0;
  183    header[2] = 0;
  184    header[3] = 0;
  185
  186    datarecv = 0;
  187    buf = NULL;
  188    len = 0;
  189    *errnop = 0;
  190
At conditional (2): "datarecv < header[0]" taking the true branch.
  191    while (datarecv < header[0]) {
  192        struct pollfd pfd;
  193        int bufrecv;
  194        int res, error;
  195
  196        pfd.fd = sss_cli_sd;
  197        pfd.events = POLLIN;
  198
  199        do {
  200            errno = 0;
  201            res = poll(&pfd, 1, SSS_CLI_SOCKET_TIMEOUT);
  202            error = errno;
  203
  204            /* If error is EINTR here, we'll try again
  205             * If it's any other error, we'll catch it
  206             * below.
  207             */
At conditional (3): "error == 4" taking the true branch.
At conditional (4): "error == 4" taking the false branch.
  208        } while (error == EINTR);
  209
  210        switch (res) {
  211        case -1:
  212            *errnop = error;
  213            break;
At conditional (5): switch case value "0" taking the true branch.
  214        case 0:
  215            *errnop = ETIME;
  216            break;
  217        case 1:
  218            if (pfd.revents & (POLLERR | POLLHUP | POLLNVAL)) {
  219                *errnop = EPIPE;
  220            }
  221            if (!(pfd.revents & POLLIN)) {
  222                *errnop = EBUSY;
  223            }
  224            break;
  225        default: /* more than one avail ?? */
  226            *errnop = EBADF;
  227            break;
  228        }
At conditional (6): "*errnop" taking the true branch.
  229        if (*errnop) {
  230            sss_cli_close_socket();
Variable "buf" going out of scope leaks the storage it points to.
  231            return NSS_STATUS_UNAVAIL;
  232        }
  233
  234        errno = 0;
  235        if (datarecv < SSS_NSS_HEADER_SIZE) {
  236            res = read(sss_cli_sd,
  237                       (char *)header + datarecv,
  238                       SSS_NSS_HEADER_SIZE - datarecv);
  239        } else {
  240            bufrecv = datarecv - SSS_NSS_HEADER_SIZE;
  241            res = read(sss_cli_sd,
  242                       (char *) buf + bufrecv,
  243                       header[0] - datarecv);
  244        }
  245        error = errno;
  246
  247        if ((res == -1) || (res == 0)) {
  248            if ((error == EINTR) || error == EAGAIN) {
  249                /* If the read was interrupted, go back through
  250                 * the loop and try again
  251                 */
  252                continue;
  253            }
  254
  255            /* Read failed.  I think the only useful thing
  256             * we can do here is just return -1 and fail
  257             * since the transaction has failed half way
  258             * through. */
  259
  260            sss_cli_close_socket();
  261            *errnop = errno;
  262            return NSS_STATUS_UNAVAIL;
  263        }
  264
  265        datarecv += res;
  266
  267        if (datarecv == SSS_NSS_HEADER_SIZE && len == 0) {
  268            /* at this point recv buf is not yet
  269             * allocated and the header has just
  270             * been read, do checks and proceed */
  271            if (header[2] != 0) {
  272                /* server side error */
  273                sss_cli_close_socket();
  274                *errnop = header[2];
  275                if (*errnop == EAGAIN) {
  276                    return NSS_STATUS_TRYAGAIN;
  277                } else {
  278                    return NSS_STATUS_UNAVAIL;
  279                }
  280            }
  281            if (header[1] != cmd) {
  282                /* wrong command id */
  283                sss_cli_close_socket();
  284                *errnop = EBADMSG;
  285                return NSS_STATUS_UNAVAIL;
  286            }
  287            if (header[0] > SSS_NSS_HEADER_SIZE) {
  288                len = header[0] - SSS_NSS_HEADER_SIZE;
Assigning: "buf" = storage returned from "malloc(len)".
Calling allocation function "malloc".
  289                buf = malloc(len);
At conditional (1): "!buf" taking the false branch.
  290                if (!buf) {
  291                    sss_cli_close_socket();
  292                    *errnop =  ENOMEM;
  293                    return NSS_STATUS_UNAVAIL;
  294                }
  295            }
  296        }
  297    }
  298
  299    *_len = len;
  300    *_buf = buf;
  301
  302    return NSS_STATUS_SUCCESS;
  303}

coverity: => 10029

Fields changed

coverity: 10029 => 10017, 10029

Fixed by 4555f34

resolution: => fixed
status: new => closed

Fields changed

rhbz: => 0

Metadata Update from @sgallagh:
- Issue assigned to sbose
- Issue set to the milestone: SSSD 1.5.0

7 years ago

SSSD is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in SSSD's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/SSSD/sssd/issues/1765

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Login to comment on this ticket.

Metadata