[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