[quagga-dev 16674] bgpd: fix file descriptor leaks in vty_close

[Evgeny Uskov]

Dear Quagga Developers,

We found a bug in bgpd which causes file descriptor leaks each time
when configuration is updated and then saved on disk. The reason is
that output file descriptor vty->wfd is not closed in vty_close
function.

This bug is very easy to reproduce. You need to
1) check a number of opened FDs (using lsof or /proc/<pid>/fd)
2) perform any update to the configuration
3) execute "write" command in "configure terminal"
4) check a number of opened FDs again

You can find a sample script (check.sh) in attachment that automates
the steps above. An example of its output:
% sudo sh check.sh $(pgrep -f ./bgpd/.libs/bgpd) localhost 2605
<telnet password>
Open FDs: 10
altering config...
Open FDs: 11
altering config 10 times...
Open FDs: 21

A simple patch that fixes the problem above is also attached.

--
| Evgeny Uskov | HLL l QRATOR
| mob.: [phone]
| skype: evgeny_uskov
| mailto: ***@qrator.net
| visit: www.qrator.net

[Balaji Gurudoss]

Hi

Thanks for the patch. what does 2 signify

if (vty->wfd > 2)
close (vty->wfd);

Thanks,
- Balaji

Post by Evgeny Uskov
Dear Quagga Developers,
We found a bug in bgpd which causes file descriptor leaks each time
when configuration is updated and then saved on disk. The reason is
that output file descriptor vty->wfd is not closed in vty_close
function.
This bug is very easy to reproduce. You need to
1) check a number of opened FDs (using lsof or /proc/<pid>/fd)
2) perform any update to the configuration
3) execute "write" command in "configure terminal"
4) check a number of opened FDs again
You can find a sample script (check.sh) in attachment that automates
% sudo sh check.sh $(pgrep -f ./bgpd/.libs/bgpd) localhost 2605
<telnet password>
Open FDs: 10
altering config...
Open FDs: 11
altering config 10 times...
Open FDs: 21
A simple patch that fixes the problem above is also attached.
--
| Evgeny Uskov | HLL l QRATOR
| mob.: [phone]
| skype: evgeny_uskov
| visit: www.qrator.net
_______________________________________________
Quagga-dev mailing list
https://lists.quagga.net/mailman/listinfo/quagga-dev

[Balaji Gurudoss]

Can we use the macros(STDERR_FILENO/STDERR_FILENO) defined in uinstd.h for
this so that its easier :)

Post by Balaji Gurudoss
Thanks for the patch. what does 2 signify
if (vty->wfd > 2)
close (vty->wfd);

the fileno: stdout = 1, stderr = 2; > 2 = regular files opened by bgpd.
Nick

[Balaji Gurudoss]

I think we should be good if we check wfd is greater than STDERR_FILENO &&
also check if the vty->wfd if its not equal to vty->fd

Post by Balaji Gurudoss
Can we use the macros(STDERR_FILENO/STDERR_FILENO) defined in uinstd.h
for this so that its easier :)

sure, but you'd need to check against STDIN_FILENO, STDOUT_FILENO and
STDERR_FILENO, which is a bit a mouthful. Maybe a comment explaining
what's going on would be more useful? It's not like any of the stdio
filenos are going to change any time soon.
Nick

[Evgeny Uskov]

Hi,

Please note that vty_close has the following code:

/* Close socket. */
if (vty->fd > 0)
close (vty->fd);
else
vty_stdio_reset ();

Condition that vty->fd is greater than zero in fact means vty->fd !=
STDIN_FILENO. I decided to use the same logic in the patch and use
numeric value 2 instead of STDERR_FILENO.

--
| Evgeny Uskov | HLL l QRATOR
| mob.: [phone]
| skype: evgeny_uskov
| mailto: ***@qrator.net
| visit: www.qrator.net

Post by Balaji Gurudoss
I think we should be good if we check wfd is greater than STDERR_FILENO &&
also check if the vty->wfd if its not equal to vty->fd

Post by Balaji Gurudoss
Can we use the macros(STDERR_FILENO/STDERR_FILENO) defined in uinstd.h
for this so that its easier :)

sure, but you'd need to check against STDIN_FILENO, STDOUT_FILENO and
STDERR_FILENO, which is a bit a mouthful. Maybe a comment explaining
what's going on would be more useful? It's not like any of the stdio
filenos are going to change any time soon.
Nick

[Paul Jakma]

Post by Evgeny Uskov
Dear Quagga Developers,
We found a bug in bgpd which causes file descriptor leaks each time
when configuration is updated and then saved on disk. The reason is
that output file descriptor vty->wfd is not closed in vty_close
function.
This bug is very easy to reproduce. You need to
1) check a number of opened FDs (using lsof or /proc/<pid>/fd)
2) perform any update to the configuration
3) execute "write" command in "configure terminal"
4) check a number of opened FDs again
You can find a sample script (check.sh) in attachment that automates
% sudo sh check.sh $(pgrep -f ./bgpd/.libs/bgpd) localhost 2605
<telnet password>
Open FDs: 10
altering config...
Open FDs: 11
altering config 10 times...
Open FDs: 21
A simple patch that fixes the problem above is also attached.

+1 from me.

Could the script be made a unit test?

regards,

[Balaji Gurudoss]

Thanks Evgeny for the patch. Applied !

Post by Evgeny Uskov
Dear Quagga Developers,

Post by Evgeny Uskov
We found a bug in bgpd which causes file descriptor leaks each time
when configuration is updated and then saved on disk. The reason is
that output file descriptor vty->wfd is not closed in vty_close
function.
This bug is very easy to reproduce. You need to
1) check a number of opened FDs (using lsof or /proc/<pid>/fd)
2) perform any update to the configuration
3) execute "write" command in "configure terminal"
4) check a number of opened FDs again
You can find a sample script (check.sh) in attachment that automates
% sudo sh check.sh $(pgrep -f ./bgpd/.libs/bgpd) localhost 2605
<telnet password>
Open FDs: 10
altering config...
Open FDs: 11
altering config 10 times...
Open FDs: 21
A simple patch that fixes the problem above is also attached.

+1 from me.
Could the script be made a unit test?
regards,
--
You will be surprised by a loud noise.
_______________________________________________
Quagga-dev mailing list
https://lists.quagga.net/mailman/listinfo/quagga-dev