From: Antonin Houska <ah@cybertec.at>
To: Tatsuo Ishii <ishii@sraoss.co.jp>
cc: pgsql-hackers@postgresql.org
Subject: Re: Memory leaks in BufFileOpenShared()
In-reply-to: <20180615.162003.2258832502225295119.t-ishii@sraoss.co.jp>
References: <11329.1529045692@localhost>
 <20180615.162003.2258832502225295119.t-ishii@sraoss.co.jp>
Comments: In-reply-to Tatsuo Ishii <ishii@sraoss.co.jp>
   message dated "Fri, 15 Jun 2018 16:20:03 +0900."
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Date: Fri, 15 Jun 2018 09:59:26 +0200
Message-ID: <16139.1529049566@localhost>
Precedence: bulk

--=-=-=
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Now I see that BufFileCreateShared() has similar problem with file->name.

More generic problem I see is that the common initialization of BufFile is
repeated a few times. The attached patch tries to improve that (it also fix=
es
the duplicate allocation of file->name).

Tatsuo Ishii <ishii@sraoss.co.jp> wrote:

> > Memory is allocated twice for "file" and "files" variables. Possible fi=
x:
> >=20
> > diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/f=
ile/buffile.c
> > index d8a18dd3dc..00f61748b3 100644
> > --- a/src/backend/storage/file/buffile.c
> > +++ b/src/backend/storage/file/buffile.c
> > @@ -277,10 +277,10 @@ BufFileCreateShared(SharedFileSet *fileset, const=
 char *name)
> >  BufFile *
> >  BufFileOpenShared(SharedFileSet *fileset, const char *name)
> >  {
> > -       BufFile    *file =3D (BufFile *) palloc(sizeof(BufFile));
> > +       BufFile    *file;
> >         char            segment_name[MAXPGPATH];
> >         Size            capacity =3D 16;
> > -       File       *files =3D palloc(sizeof(File) * capacity);
> > +       File       *files;
> >         int                     nfiles =3D 0;
> >=20=20
> >         file =3D (BufFile *) palloc(sizeof(BufFile));
>=20
> Good catch. Thanks.
>=20
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp
>=20

--=20
Antonin Houska
Cybertec Sch=C3=B6nig & Sch=C3=B6nig GmbH
Gr=C3=B6hrm=C3=BChlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com


--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment; filename=buffile_refactor.patch

diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
index d8a18dd3dc..e345d008b5 100644
--- a/src/backend/storage/file/buffile.c
+++ b/src/backend/storage/file/buffile.c
@@ -99,6 +99,7 @@ struct BufFile
 	char		buffer[BLCKSZ];
 };
 
+static BufFile *makeBufFileCommon(int nfiles);
 static BufFile *makeBufFile(File firstfile);
 static void extendBufFile(BufFile *file);
 static void BufFileLoadBuffer(BufFile *file);
@@ -106,21 +107,16 @@ static void BufFileDumpBuffer(BufFile *file);
 static int	BufFileFlush(BufFile *file);
 static File MakeNewSharedSegment(BufFile *file, int segment);
 
-
 /*
- * Create a BufFile given the first underlying physical file.
- * NOTE: caller must set isInterXact if appropriate.
+ * Create BufFile and perform the common initialization.
  */
 static BufFile *
-makeBufFile(File firstfile)
+makeBufFileCommon(int nfiles)
 {
 	BufFile    *file = (BufFile *) palloc(sizeof(BufFile));
 
-	file->numFiles = 1;
-	file->files = (File *) palloc(sizeof(File));
-	file->files[0] = firstfile;
-	file->offsets = (off_t *) palloc(sizeof(off_t));
-	file->offsets[0] = 0L;
+	file->numFiles = nfiles;
+	file->offsets = (off_t *) palloc0(sizeof(off_t) * nfiles);
 	file->isInterXact = false;
 	file->dirty = false;
 	file->resowner = CurrentResourceOwner;
@@ -128,6 +124,21 @@ makeBufFile(File firstfile)
 	file->curOffset = 0L;
 	file->pos = 0;
 	file->nbytes = 0;
+
+	return file;
+}
+
+/*
+ * Create a BufFile given the first underlying physical file.
+ * NOTE: caller must set isInterXact if appropriate.
+ */
+static BufFile *
+makeBufFile(File firstfile)
+{
+	BufFile    *file = makeBufFileCommon(1);
+
+	file->files = (File *) palloc(sizeof(File));
+	file->files[0] = firstfile;
 	file->readOnly = false;
 	file->fileset = NULL;
 	file->name = NULL;
@@ -246,23 +257,12 @@ BufFileCreateShared(SharedFileSet *fileset, const char *name)
 {
 	BufFile    *file;
 
-	file = (BufFile *) palloc(sizeof(BufFile));
+	file = makeBufFileCommon(1);
 	file->fileset = fileset;
 	file->name = pstrdup(name);
-	file->numFiles = 1;
 	file->files = (File *) palloc(sizeof(File));
 	file->files[0] = MakeNewSharedSegment(file, 0);
-	file->offsets = (off_t *) palloc(sizeof(off_t));
-	file->offsets[0] = 0L;
-	file->isInterXact = false;
-	file->dirty = false;
-	file->resowner = CurrentResourceOwner;
-	file->curFile = 0;
-	file->curOffset = 0L;
-	file->pos = 0;
-	file->nbytes = 0;
 	file->readOnly = false;
-	file->name = pstrdup(name);
 
 	return file;
 }
@@ -286,4 +286,3 @@
-	file = (BufFile *) palloc(sizeof(BufFile));
 	files = palloc(sizeof(File) * capacity);
 
 	/*
@@ -317,16 +316,8 @@ BufFileOpenShared(SharedFileSet *fileset, const char *name)
 				(errcode_for_file_access(),
 				 errmsg("could not open BufFile \"%s\"", name)));
 
-	file->numFiles = nfiles;
+	file = makeBufFileCommon(nfiles);
 	file->files = files;
-	file->offsets = (off_t *) palloc0(sizeof(off_t) * nfiles);
-	file->isInterXact = false;
-	file->dirty = false;
-	file->resowner = CurrentResourceOwner;	/* Unused, can't extend */
-	file->curFile = 0;
-	file->curOffset = 0L;
-	file->pos = 0;
-	file->nbytes = 0;
 	file->readOnly = true;		/* Can't write to files opened this way */
 	file->fileset = fileset;
 	file->name = pstrdup(name);

--=-=-=--