[SSSD] [PATCH] LDAP: Fix leak of file descriptors

Lukas Slebodnik lslebodn at redhat.com
Thu Nov 5 07:50:13 UTC 2015


On (04/11/15 20:39), Jakub Hrozek wrote:
>On Wed, Nov 04, 2015 at 03:22:20PM +0100, Lukas Slebodnik wrote:
>> >Do you think it would be safer to initialize state->sd to -1 after the
>> >state is created and only close state->sd if it's > 1? I think if the
>> >socket() call would fail, then state->sd was 0 (because tevent uses
>> >talloc_zero to create the state) and then we might attempt to close
>> >stdin..
>> The state->sd would be initialized either by socket or in
>> part if HAVE_LDAP_INIT_FD is not defined to -1.
>> 
>> It could be 0 only in case if function socket returned 0.
>> which is very unlikely. The file descriptor 0 would be already
>> occupied by other file. But we shoudl not rely on this.
>> 
>> and on the other hand it's nicer to test for -1.
>
>Thanks. I still think it would be better to initialize the socket FD to
>-1 after the state is created.
static analyzers would comply that state->sd it was set but not used.

   334      req = tevent_req_create(mem_ctx, &state, struct sss_ldap_init_state);
   335      if (req == NULL) {
   336          DEBUG(SSSDBG_CRIT_FAILURE, "tevent_req_create failed.\n");
   337          return NULL;
   338      }
   339  
   340      talloc_set_destructor((TALLOC_CTX *)state, sss_ldap_init_state_destructor);
   341  
   342      state->ldap = NULL;
   343      state->uri = uri;
   344  
   345  #ifdef HAVE_LDAP_INIT_FD
   346      struct tevent_req *subreq;
   347      struct timeval tv;
   348  
   349      state->sd = socket(addr->ss_family, SOCK_STREAM, 0);
            ^^^^^^^^^
            //initialized here
   350      if (state->sd == -1) {
                ^^^^^^^^^
                // in case of error it will be -1
   351          ret = errno;
   352          DEBUG(SSSDBG_CRIT_FAILURE,
   353                "socket failed [%d][%s].\n", ret, strerror(ret));
   354          goto fail;
   355      }

//snip
   390  fail:
   391      tevent_req_error(req, ret);
   392  #else
   393      DEBUG(SSSDBG_MINOR_FAILURE, "ldap_init_fd not available, "
   394                "will use ldap_initialize with uri [%s].\n", uri);
   395      state->sd = -1;
            ^^^^^^^^^
            //initialized here to -1

   396      ret = ldap_initialize(&state->ldap, uri);
   397      if (ret == LDAP_SUCCESS) {
   398          tevent_req_done(req);
   399      } else {
   400          DEBUG(SSSDBG_CRIT_FAILURE,
   401                "ldap_initialize failed [%s].\n", sss_ldap_err2string(ret));
   402          if (ret == LDAP_SERVER_DOWN) {
   403              tevent_req_error(req, ETIMEDOUT);
   404          } else {
   405              tevent_req_error(req, EIO);
   406          }
   407      }
   408  #endif
   409  
   410      tevent_req_post(req, ev);



>But if you feel too strongly, I won't oppose. I just think if the code
>was confusing to me now it might be confusing for other developers
>trying to find bugs in the future...

So do we want to add new warnig from static analyzers?

LS


More information about the sssd-devel mailing list