Bug #2159

src/mod_cgi.c leaks opened file descriptors

Added by rata over 4 years ago. Updated over 4 years ago.

Status:FixedStart date:2010-01-30
Priority:NormalDue date:
Assignee:-% Done:

100%

Category:mod_cgi
Target version:-
Missing in 1.5.x:No

Description

When reporting http://redmine.lighttpd.net/issues/2158 I noticed that in file src/mod_cgi.c the function cgi_create_env leaks opened file descriptors in other cases (no just when fork() fails).

For example, if the first call to pipe() works ok, but the second one fails, the fds created in the first call to pipe() are leaked. Just closing the fds before the return statement would work, but perhaps its more convenient to add a label and a goto, because that would avoid the duplicated code on the error paths.

I'm not sure if there are more cases when they are leaked (I think I found more but I don't remember now). If I have time I will check again and report back, but anyways I wanted to report this since most probably I will forget to check again :)

Thanks a lot,
Rodrigo

Associated revisions

Revision 2706
Added by stbuehler over 4 years ago

Fix fd leaks in mod_cgi (fds not closed on pipe/fork failures, found by Rodrigo, fixes #2158, #2159)

History

#1 Updated by stbuehler over 4 years ago

  • Status changed from New to Fixed
  • % Done changed from 0 to 100

Applied in changeset r2706.

#2 Updated by rata over 4 years ago

Sorry for the delay, I've re checked and I think no other fd leak is left on
that function now.

The first two returns of the function are without anything allocated. The third
one is with the pipe call failure which is already fixed.

Then we have the switch where we fork. In case of error, the fds are closed. If
the default case is executed, we close at the beginning from_cgi_fds [1] and
to_cgi_fds [0]. So we still need to close from_cgi_fds [0] and to_cgi_fds [1].
Suppose the first if (if (con->request.content_length) is false. Then just after
that if, we close to_cgi_fds [1]. So the only one missing is from_cgi_fds [0] but
its is used with "fdevent_register" and lets assume it closes it (I've checked
it quicky and it seems ok) and also if we fail to use it with "fdevent_register"
it (the fd) is closed.
Also if the "if" statement (if (con->request.content_length)) is true, then all
uses inside that if of "from_cgi_fds [0]" are a close() to that fd before a
return statement, which is ok. And all uses of "to_cgi_fds [1]" are the same of
"from_cgi_fds [0]" except that it also write() to it. So it should be all ok if
I'm not missing something.

Also, I just noted that the default case before is the normal/regular path, so
its expected that there is no leak there (it would be noticied before :). So
when fork returns 0 (also the normal/regular path) all calls to close() are done
inconditionally and they seem correct (and I have not detected and fd leaks when
running normally). So I think that fds are not leaked in this function =)

Thanks a lot,
Rodrigo

Also available in: Atom