neat_free_candidate: uses already closed file descriptor

[bagder]

candidate->pollable_socket->fd is closed on neat_core.c:537 (and not assigned to anything else)

It is then subsequently wrongly accessed and used on line 550 and onwards.

Coverity CID 1452602

[karlgrin]

@bagder To me it seems that it is enough to assign 'candidate->pollable_socket->fd' to '-1' after 'close(candidate->pollable_socket->fd)'. Or, is it something I am missing here?

[bagder]

I don't understand the logic in this function so I'm not sure.

if (candidate->pollable_socket->fd) { in itself seems wrong since zero is a valid file descriptor, while -1 is typically used for an already-closed descriptor. That should probably rather be:

    if (candidate->pollable_socket->fd != -1) {
        close(candidate->pollable_socket->fd);
        candidate->pollable_socket->fd = -1;
    }

... but if we do that, the check further down that checks if (candidate->pollable_socket->fd == -1) would be totally superfluous as it would then always be true so the two else clauses below that are basically dead code!

[karlgrin]

@bagder I have removed the lines

if (candidate->pollable_socket->fd != -1) {
           close(candidate->pollable_socket->fd);
           candidate->pollable_socket->fd = -1;
    }

From my understanding, we should anyway close the file descriptor by entering the

... else if ( !uv_is_closing((uv_handle_t *) candidate->pollable_socket->handle)) {
   ...
   }

If you agree, we could close this issue.

[bagder]

The new code:

    if (candidate->pollable_socket->fd) {
        close(candidate->pollable_socket->fd);
    }

.. still doesn't check for != -1, but more importantly it doesn't set it to -1 after it closes the file descriptor so it will not match the correct check further down in the same function.