#1233 Memory leak in sss_sudo_send_recv_generic
Closed: Fixed None Opened 10 years ago by sgallagh.

There are two bugs here.

  1. {{{sss_sudo_send_recv_generic()}}} Should be testing the return value of sss_sudo_make_request(), not errnop directly.
  2. For some reason, it's possible for {{{sss_cli_recv_rep()}}} to return errnop != EOK and an allocated buffer at the same time. This needs investigation.

    39static int sss_sudo_send_recv_generic(enum sss_cli_command command,
    40 struct sss_cli_req_data request,
    41 uint32_t
    42 struct sss_sudo_result _result)
    44 uint8_t
    reply_buf = NULL;
    45 size_t reply_len = 0;
    46 int errnop = 0;
    47 int ret = 0;
    49 /
    send query and receive response */
    51 errnop = 0;
    CID 12576: Resource leak (RESOURCE_LEAK)Calling allocation function "sss_sudo_make_request" on "reply_buf". [show details]
    52 ret = sss_sudo_make_request(command, request,
    53 &reply_buf, &reply_len, &errnop);
    At conditional (1): "errnop != 0" taking the true branch.
    54 if (errnop != EOK) {
    Variable "reply_buf" going out of scope leaks the storage it points to.
    55 return errnop;
    56 }

The same issue exists in {{{_sss_setautomntent()}}} and {{{_sss_getautomntent_r()}}}

coverity: 12576 => 12576, 12572, 12573

I don't think 2) is right. All the code paths in {{{sss_cli_recv_rep()}}} that set errnop to != end in the "fail" label that frees the allocated buffer. errnop is also initalized to 0 at the beginning of {{{sss_cli_recv_rep()}}}.

That said, I think we should convert the {{{sss_cli_recv_rep()}}} consumers (that is all {{{sss_$SERVICE_make_request()}}} functions) to use a single return code rather than status and errnop. One possible exception being {{{sss_nss_make_request}}}, because the NSS client uses errnop to set errno and glibc treats some error conditions as recoverable (such as status=TRYAGAIN && errno=ERANGE which would cause glibc to retry with a bigger buffer).

Fields changed

milestone: NEEDS_TRIAGE => SSSD 1.9.0
rhbz: => 0

Fields changed

owner: somebody => pbrezina
status: new => assigned

I agree that 2) is a false positive.
I went through the Coverity output.

I don't see any way how the condition #36 (datarecv < header[0]) can be false when #30 (datarecv == HEADER_SIZE) and #34 (header[0] > HEADER_SIZE) are true. And any way if this would happen the errnop would still be 0 therefore the result buffer would be freed.

Fixed by:

resolution: => fixed
status: assigned => closed

Fields changed

milestone: SSSD 1.9.0 => SSSD 1.9.0 beta 1

Metadata Update from @sgallagh:
- Issue assigned to pbrezina
- Issue set to the milestone: SSSD 1.9.0 beta 1

5 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/2275

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.