#1233 Memory leak in sss_sudo_send_recv_generic
Closed: Fixed None Opened 7 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

2 years ago

Login to comment on this ticket.