Thread: Memory leaks in BufFileOpenShared()

Memory leaks in BufFileOpenShared()

From

Antonin Houska

Date:

15 June 2018, 12:54:52

Memory is allocated twice for "file" and "files" variables. Possible fix:

diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/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 = (BufFile *) palloc(sizeof(BufFile));
+       BufFile    *file;
        char            segment_name[MAXPGPATH];
        Size            capacity = 16;
-       File       *files = palloc(sizeof(File) * capacity);
+       File       *files;
        int                     nfiles = 0;

        file = (BufFile *) palloc(sizeof(BufFile));


--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com

Re: Memory leaks in BufFileOpenShared()

From

Tatsuo Ishii

Date:

15 June 2018, 13:20:03

> Memory is allocated twice for "file" and "files" variables. Possible fix:
> 
> diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/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 = (BufFile *) palloc(sizeof(BufFile));
> +       BufFile    *file;
>         char            segment_name[MAXPGPATH];
>         Size            capacity = 16;
> -       File       *files = palloc(sizeof(File) * capacity);
> +       File       *files;
>         int                     nfiles = 0;
>  
>         file = (BufFile *) palloc(sizeof(BufFile));

Good catch. Thanks.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Re: Memory leaks in BufFileOpenShared()

From

Antonin Houska

Date:

15 June 2018, 13:59:26

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 fixes
the duplicate allocation of file->name).

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

> > Memory is allocated twice for "file" and "files" variables. Possible fix:
> >
> > diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/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 = (BufFile *) palloc(sizeof(BufFile));
> > +       BufFile    *file;
> >         char            segment_name[MAXPGPATH];
> >         Size            capacity = 16;
> > -       File       *files = palloc(sizeof(File) * capacity);
> > +       File       *files;
> >         int                     nfiles = 0;
> >
> >         file = (BufFile *) palloc(sizeof(BufFile));
>
> Good catch. Thanks.
>
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp
>

--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com

Attachment

buffile_refactor.patch

Re: Memory leaks in BufFileOpenShared()

From

Tatsuo Ishii

Date:

15 June 2018, 14:22:08

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

Right.

> 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 fixes
> the duplicate allocation of file->name).

The changes were made by this commit to add infrastructure for sharing
temporary files between backends, according to the author (Thomas
Munro).

https://git.postgresql.org/pg/commitdiff/dc6c4c9dc2a111519b76b22daaaac86c5608223b

Your proposal looks reasonable but I would like to hear from Thomas's
opinion as well.

Thomas?

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Re: Memory leaks in BufFileOpenShared()

From

Antonin Houska

Date:

15 June 2018, 14:42:33

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

> The changes were made by this commit to add infrastructure for sharing
> temporary files between backends, according to the author (Thomas
> Munro).
>
> https://git.postgresql.org/pg/commitdiff/dc6c4c9dc2a111519b76b22daaaac86c5608223b
>
> Your proposal looks reasonable but I would like to hear from Thomas's
> opinion as well.

o.k. He can actually have different feeling about details, e.g. if a new
function makeBufFileCommon() should be introduced or if makeBufFile() should
do the common settings. In the latter case, BufFileCreateTemp() would have to
do more work than it does now.

--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com

Re: Memory leaks in BufFileOpenShared()

From

Thomas Munro

Date:

15 June 2018, 14:54:41

On Fri, Jun 15, 2018 at 8:42 PM, Antonin Houska <ah@cybertec.at> wrote:
> Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
>
>> The changes were made by this commit to add infrastructure for sharing
>> temporary files between backends, according to the author (Thomas
>> Munro).
>>
>> https://git.postgresql.org/pg/commitdiff/dc6c4c9dc2a111519b76b22daaaac86c5608223b
>>
>> Your proposal looks reasonable but I would like to hear from Thomas's
>> opinion as well.
>
> o.k. He can actually have different feeling about details, e.g. if a new
> function makeBufFileCommon() should be introduced or if makeBufFile() should
> do the common settings. In the latter case, BufFileCreateTemp() would have to
> do more work than it does now.

Thanks for finding these accidental duplications, and to Ishii-san for
committing the fix.  Oops.

+1 for this refactoring.

-- 
Thomas Munro
http://www.enterprisedb.com

Re: Memory leaks in BufFileOpenShared()

From

Tatsuo Ishii

Date:

15 June 2018, 15:07:08

Hi Thomas,

> On Fri, Jun 15, 2018 at 8:42 PM, Antonin Houska <ah@cybertec.at> wrote:
>> Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
>>
>>> The changes were made by this commit to add infrastructure for sharing
>>> temporary files between backends, according to the author (Thomas
>>> Munro).
>>>
>>> https://git.postgresql.org/pg/commitdiff/dc6c4c9dc2a111519b76b22daaaac86c5608223b
>>>
>>> Your proposal looks reasonable but I would like to hear from Thomas's
>>> opinion as well.
>>
>> o.k. He can actually have different feeling about details, e.g. if a new
>> function makeBufFileCommon() should be introduced or if makeBufFile() should
>> do the common settings. In the latter case, BufFileCreateTemp() would have to
>> do more work than it does now.
> 
> Thanks for finding these accidental duplications, and to Ishii-san for
> committing the fix.  Oops.
> 
> +1 for this refactoring.

Thanks for confirming. I will go ahead commit/push the patch.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Re: Memory leaks in BufFileOpenShared()

From

Tatsuo Ishii

Date:

16 June 2018, 11:44:20

>> Thanks for finding these accidental duplications, and to Ishii-san for
>> committing the fix.  Oops.
>> 
>> +1 for this refactoring.
> 
> Thanks for confirming. I will go ahead commit/push the patch.

Done.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp