MIME-Version: 1.0
References: <62bbe34d-2315-4b42-b768-56d901aa83e1@gmail.com>
 <1302471.1773940095@sss.pgh.pa.us>
In-Reply-To: <1302471.1773940095@sss.pgh.pa.us>
From: Jianghua Yang <yjhjstz@gmail.com>
Date: Thu, 19 Mar 2026 10:19:10 -0700
Message-ID: 
 <CAAZLFmR-S3tyN2xC1K0jZ3T13JZD=_36mYu+0WKCKs1wEROuLw@mail.gmail.com>
Subject: Re: [PATCH] Fix fd leak in pg_dump compression backends when
 dup()+fdopen() fails
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@lists.postgresql.org
Content-Type: multipart/alternative; boundary="0000000000006d85aa064d63c6a3"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAZLFmR-S3tyN2xC1K0jZ3T13JZD%3D_36mYu%2B0WKCKs1wEROuLw%40mail.gmail.com>
Precedence: bulk

--0000000000006d85aa064d63c6a3
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

You're correct.  All callers invoke pg_fatal() on failure, so the
  process exits immediately and the OS reclaims the fd.  There is no
  live bug worth back-patching on those grounds.

  That said, the patch does fix a real diagnostic problem.  In the
  original code, when dup() fails with EMFILE, the -1 return value is
  passed directly to fdopen(), which fails with EBADF.  The user sees:

    pg_dump: error: could not open output file: Bad file descriptor

  which is misleading -- the actual cause is fd exhaustion, not a bad
  descriptor.  With the patch, errno is preserved correctly, so the
  message becomes:

    pg_dump: error: could not open output file: Too many open files

  which gives the user actionable information.

  I'm happy to limit this to HEAD only if back-patching is not
  warranted.

  Regards,
  Jianghua Yang

Tom Lane <tgl@sss.pgh.pa.us> =E4=BA=8E2026=E5=B9=B43=E6=9C=8819=E6=97=A5=E5=
=91=A8=E5=9B=9B 10:08=E5=86=99=E9=81=93=EF=BC=9A

> Jianghua Yang <yjhjstz@gmail.com> writes:
> >    =3D=3D The Bug =3D=3D
>
> >    All four compression open functions use this pattern when an existin=
g
> >    file descriptor is passed in:
>
> >        if (fd >=3D 0)
> >            fp =3D fdopen(dup(fd), mode);   /* or gzdopen() */
>
> >        if (fp =3D=3D NULL)
> >            return false;                 /* dup'd fd is leaked here */
>
> >    The problem is that dup(fd) and fdopen()/gzdopen() are two separate
> >    steps, and their failure modes must be handled independently:
>
> Hmm.  You're right that we could leak the dup'd FD, but would it matter?
> I'm pretty sure all these programs will just exit immediately on
> failure.
>
> I'm not averse to improving the code, but I'm not sure there is
> a live bug worth back-patching.
>
>                         regards, tom lane
>

--0000000000006d85aa064d63c6a3
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">You&#39;re correct.=C2=A0 All callers invoke pg_fatal() on=
 failure, so the<br>=C2=A0 process exits immediately and the OS reclaims th=
e fd.=C2=A0 There is no<br>=C2=A0 live bug worth back-patching on those gro=
unds.<br><br>=C2=A0 That said, the patch does fix a real diagnostic problem=
.=C2=A0 In the<br>=C2=A0 original code, when dup() fails with EMFILE, the -=
1 return value is<br>=C2=A0 passed directly to fdopen(), which fails with E=
BADF.=C2=A0 The user sees:<br><br>=C2=A0 =C2=A0 pg_dump: error: could not o=
pen output file: Bad file descriptor<br><br>=C2=A0 which is misleading -- t=
he actual cause is fd exhaustion, not a bad<br>=C2=A0 descriptor.=C2=A0 Wit=
h the patch, errno is preserved correctly, so the<br>=C2=A0 message becomes=
:<br><br>=C2=A0 =C2=A0 pg_dump: error: could not open output file: Too many=
 open files<br><br>=C2=A0 which gives the user actionable information.<br><=
br>=C2=A0 I&#39;m happy to limit this to HEAD only if back-patching is not<=
br>=C2=A0 warranted.<br><br>=C2=A0 Regards,<br>=C2=A0 Jianghua Yang</div><b=
r><div class=3D"gmail_quote gmail_quote_container"><div dir=3D"ltr" class=
=3D"gmail_attr">Tom Lane &lt;<a href=3D"mailto:tgl@sss.pgh.pa.us">tgl@sss.p=
gh.pa.us</a>&gt; =E4=BA=8E2026=E5=B9=B43=E6=9C=8819=E6=97=A5=E5=91=A8=E5=9B=
=9B 10:08=E5=86=99=E9=81=93=EF=BC=9A<br></div><blockquote class=3D"gmail_qu=
ote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,20=
4);padding-left:1ex">Jianghua Yang &lt;<a href=3D"mailto:yjhjstz@gmail.com"=
 target=3D"_blank">yjhjstz@gmail.com</a>&gt; writes:<br>
&gt;=C2=A0 =C2=A0 =3D=3D The Bug =3D=3D<br>
<br>
&gt;=C2=A0 =C2=A0 All four compression open functions use this pattern when=
 an existing<br>
&gt;=C2=A0 =C2=A0 file descriptor is passed in:<br>
<br>
&gt;=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (fd &gt;=3D 0)<br>
&gt;=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 fp =3D fdopen(dup(fd), mode);=
=C2=A0 =C2=A0/* or gzdopen() */<br>
<br>
&gt;=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (fp =3D=3D NULL)<br>
&gt;=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return false;=C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* dup&#39;d fd is leaked h=
ere */<br>
<br>
&gt;=C2=A0 =C2=A0 The problem is that dup(fd) and fdopen()/gzdopen() are tw=
o separate<br>
&gt;=C2=A0 =C2=A0 steps, and their failure modes must be handled independen=
tly:<br>
<br>
Hmm.=C2=A0 You&#39;re right that we could leak the dup&#39;d FD, but would =
it matter?<br>
I&#39;m pretty sure all these programs will just exit immediately on<br>
failure.<br>
<br>
I&#39;m not averse to improving the code, but I&#39;m not sure there is<br>
a live bug worth back-patching.<br>
<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 regards, tom lane<br>
</blockquote></div>

--0000000000006d85aa064d63c6a3--