Mailing List Archive: bgpd: fix file descriptor leaks in vty

bgpd: fix file descriptor leaks in vty_close

Nov 24, 2017, 5:21 AM

Post #1 of 7 (958 views)

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.: +7 916 319 33 20
| skype: evgeny_uskov
| mailto: eu@qrator.net
| visit: www.qrator.net

Re: bgpd: fix file descriptor leaks in vty_close [ In reply to ]

balajig81 at gmail

Nov 26, 2017, 8:15 AM

Post #2 of 7 (955 views)

Permalink

Hi

Thanks for the patch. what does 2 signify

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

Thanks,
- Balaji

On Fri, Nov 24, 2017 at 6:51 PM, Evgeny Uskov <eu@qrator.net> wrote:

> 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.: +7 916 319 33 20
> | skype: evgeny_uskov
> | mailto: eu@qrator.net
> | visit: www.qrator.net
>
> _______________________________________________
> Quagga-dev mailing list
> Quagga-dev@lists.quagga.net
> https://lists.quagga.net/mailman/listinfo/quagga-dev
>

Re: bgpd: fix file descriptor leaks in vty_close [ In reply to ]

balajig81 at gmail

Nov 26, 2017, 8:49 AM

Post #3 of 7 (955 views)

Permalink

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

On Sun, Nov 26, 2017 at 10:12 PM, Nick Hilliard <nick@inex.ie> wrote:

> Balaji Gurudoss wrote:
> > 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
>
>

Re: bgpd: fix file descriptor leaks in vty_close [ In reply to ]

balajig81 at gmail

Nov 26, 2017, 9:15 AM

Post #4 of 7 (955 views)

Permalink

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

On Sun, Nov 26, 2017 at 10:34 PM, Nick Hilliard <nick@inex.ie> wrote:

> Balaji Gurudoss wrote:
> > 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
>
>

Re: bgpd: fix file descriptor leaks in vty_close [ In reply to ]

eu at qrator

Nov 27, 2017, 4:48 AM

Post #5 of 7 (952 views)

Permalink

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.: +7 916 319 33 20
| skype: evgeny_uskov
| mailto: eu@qrator.net
| visit: www.qrator.net

On Sun, Nov 26, 2017 at 8:15 PM, Balaji Gurudoss <balajig81@gmail.com> wrote:
> 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
>
>
>
> On Sun, Nov 26, 2017 at 10:34 PM, Nick Hilliard <nick@inex.ie> wrote:
>>
>> Balaji Gurudoss wrote:
>> > 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
>>
>
_______________________________________________
Quagga-dev mailing list
Quagga-dev@lists.quagga.net
https://lists.quagga.net/mailman/listinfo/quagga-dev

Re: bgpd: fix file descriptor leaks in vty_close [ In reply to ]

paul at jakma

Nov 27, 2017, 2:54 PM

Post #6 of 7 (952 views)

Permalink

On Fri, 24 Nov 2017, Evgeny Uskov wrote:

> 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.

+1 from me.

Could the script be made a unit test?

regards,
--
Paul Jakma | paul@jakma.org | @pjakma | Key ID: 0xD86BF79464A2FF6A
Fortune:
You will be surprised by a loud noise.
_______________________________________________
Quagga-dev mailing list
Quagga-dev@lists.quagga.net
https://lists.quagga.net/mailman/listinfo/quagga-dev

Re: bgpd: fix file descriptor leaks in vty_close [ In reply to ]

balajig81 at gmail

Dec 2, 2017, 6:45 AM

Post #7 of 7 (936 views)

Permalink

Thanks Evgeny for the patch. Applied !

On Tue, Nov 28, 2017 at 4:24 AM, Paul Jakma <paul@jakma.org> wrote:

> On Fri, 24 Nov 2017, Evgeny Uskov wrote:
>
> 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.
>>
>
> +1 from me.
>
> Could the script be made a unit test?
>
> regards,
> --
> Paul Jakma | paul@jakma.org | @pjakma | Key ID: 0xD86BF79464A2FF6A
> Fortune:
> You will be surprised by a loud noise.
>
> _______________________________________________
> Quagga-dev mailing list
> Quagga-dev@lists.quagga.net
> https://lists.quagga.net/mailman/listinfo/quagga-dev
>

Mailing List Archive

Mailing List Archive

Attached Files: