Fix resource leak in iptables/xtables-restore.c

Fix resource leak in iptables/xtables-restore.c

[周恺航]

The function xtables_restore_main opens a file stream p.in but fails to close it before returning. This leads to a resource leak as the file descriptor remains open.


Signed-off-by: Kaihang Zhou <22321077@zju.edu.cn>

---
 iptables/xtables-restore.c | 1 +
 1 file changed, 1 insertion(+)


diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c

index e7802b9e..f09ab7ee 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -381,6 +381,7 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])
                break;
        default:
                fprintf(stderr, "Unknown family %d\n", family);
+               fclose(p.in);
                return 1;
        }


--

2.43.0

Re: Fix resource leak in iptables/xtables-restore.c

[Phil Sutter]

Hi,

On Mon, May 12, 2025 at 03:10:47PM +0800, 周恺航 wrote:
> The function xtables_restore_main opens a file stream p.in but fails to close it before returning. This leads to a resource leak as the file descriptor remains open.
> 
> 
> Signed-off-by: Kaihang Zhou <22321077@zju.edu.cn>
> 
> ---
>  iptables/xtables-restore.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> 
> diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
> 
> index e7802b9e..f09ab7ee 100644
> --- a/iptables/xtables-restore.c
> +++ b/iptables/xtables-restore.c
> @@ -381,6 +381,7 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])
>                 break;
>         default:
>                 fprintf(stderr, "Unknown family %d\n", family);
> +               fclose(p.in);
>                 return 1;
>         }

Since this is not the only error path which leaves p.in open (eight
lines below is the next one for instance), why fix this one in
particular and leave the other ones in place?

Cheers, Phil

Re: Re: Fix resource leak in iptables/xtables-restore.c

[周恺航]

> -----Original Message-----


> From: "Phil Sutter" <phil@nwl.cc>

> Sent: Monday, May 12, 2025 18:23:28
> To: 周恺航 <22321077@zju.edu.cn>
> Cc: netfilter-devel@vger.kernel.org
> Subject: Re: Fix resource leak in iptables/xtables-restore.cc
> 
> Hi,
> 
> On Mon, May 12, 2025 at 03:10:47PM +0800, 周恺航 wrote:
> > The function xtables_restore_main opens a file stream p.in but fails to close it before returning. This leads to a resource leak as the file descriptor remains open.
> > 
> > 
> > Signed-off-by: Kaihang Zhou <22321077@zju.edu.cn>
> > 
> > ---
> >  iptables/xtables-restore.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > 
> > diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
> > 
> > index e7802b9e..f09ab7ee 100644
> > --- a/iptables/xtables-restore.c
> > +++ b/iptables/xtables-restore.c
> > @@ -381,6 +381,7 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])
> >                 break;
> >         default:
> >                 fprintf(stderr, "Unknown family %d\n", family);
> > +               fclose(p.in);
> >                 return 1;
> >         }
> 
> Since this is not the only error path which leaves p.in open (eight
> lines below is the next one for instance), why fix this one in
> particular and leave the other ones in place?
> 
> Cheers, Phil

At first, I thought that not closing the file handle before the return was more serious, and that when exit terminates the program, the system might automatically reclaim resources. But it's obvious that this understanding is wrong. Both are bad programming habits and may lead to problems in resource management and program stability. I've revised the patch.Thank you.



Signed-off-by: Kaihang Zhou <22321077@zju.edu.cn>


---

 iptables/xtables-restore.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c


index e7802b9e..62ee68fc 100644

--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -381,6 +381,7 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])
                break;
        default:
                fprintf(stderr, "Unknown family %d\n", family);
+               fclose(p.in);
                return 1;
        }

@@ -389,6 +390,7 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])


                                xtables_globals.program_name,

                                xtables_globals.program_version,
                                strerror(errno));
+               fclose(p.in);
                exit(EXIT_FAILURE);
        }
        h.noflush = noflush;


------------------------------
2.43.0

Re: Re: Fix resource leak in iptables/xtables-restore.c

[Phil Sutter]

On Tue, May 13, 2025 at 01:20:05PM +0800, 周恺航 wrote:
> 
> 
> 
> > -----Original Message-----
> 
> 
> > From: "Phil Sutter" <phil@nwl.cc>
> 
> > Sent: Monday, May 12, 2025 18:23:28
> > To: 周恺航 <22321077@zju.edu.cn>
> > Cc: netfilter-devel@vger.kernel.org
> > Subject: Re: Fix resource leak in iptables/xtables-restore.cc
> > 
> > Hi,
> > 
> > On Mon, May 12, 2025 at 03:10:47PM +0800, 周恺航 wrote:
> > > The function xtables_restore_main opens a file stream p.in but fails to close it before returning. This leads to a resource leak as the file descriptor remains open.
> > > 
> > > 
> > > Signed-off-by: Kaihang Zhou <22321077@zju.edu.cn>
> > > 
> > > ---
> > >  iptables/xtables-restore.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > 
> > > diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
> > > 
> > > index e7802b9e..f09ab7ee 100644
> > > --- a/iptables/xtables-restore.c
> > > +++ b/iptables/xtables-restore.c
> > > @@ -381,6 +381,7 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])
> > >                 break;
> > >         default:
> > >                 fprintf(stderr, "Unknown family %d\n", family);
> > > +               fclose(p.in);
> > >                 return 1;
> > >         }
> > 
> > Since this is not the only error path which leaves p.in open (eight
> > lines below is the next one for instance), why fix this one in
> > particular and leave the other ones in place?
> > 
> > Cheers, Phil
> 
> At first, I thought that not closing the file handle before the return was more serious, and that when exit terminates the program, the system might automatically reclaim resources. But it's obvious that this understanding is wrong. Both are bad programming habits and may lead to problems in resource management and program stability. I've revised the patch.Thank you.

I was not listing all problematic cases but merely giving an example.
Another one is the exit() call in xtables_restore_parse() or all calls
to xtables_error() in various spots. If you want to avoid p.in remaining
open upon program exit in error paths, please submit a patch which
addresses all cases.

Thanks, Phil