Tighten use of OpenTransientFile and CloseTransientFile

This fixes two sets of issues related to the use of transient files in
the backend:
1) OpenTransientFile() has been used in some code paths with read-write
flags while read-only is sufficient, so switch those calls to be
read-only where necessary.  These have been reported by Joe Conway.
2) When opening transient files, it is up to the caller to close the
file descriptors opened.  In error code paths, CloseTransientFile() gets
called to clean up things before issuing an error.  However in normal
exit paths, a lot of callers of CloseTransientFile() never actually
reported errors, which could leave a file descriptor open without
knowing about it.  This is an issue I complained about a couple of
times, but never had the courage to write and submit a patch, so here we
go.

Note that one frontend code path is impacted by this commit so as an
error is issued when fetching control file data, making backend and
frontend to be treated consistently.

Reported-by: Joe Conway, Michael Paquier
Author: Michael Paquier
Reviewed-by: Álvaro Herrera, Georgios Kokolatos, Joe Conway
Discussion: https://postgr.es/m/20190301023338.GD1348@paquier.xyz
Discussion: https://postgr.es/m/c49b69ec-e2f7-ff33-4f17-0eaa4f2cef27@joeconway.com

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 9905593661e9015201ca2842993bf23b88248033..7b39283c899d767e6ff57960a8ee97a20684785c 100644 (file)
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1991,7 +1991,10 @@ qtext_load_file(Size *buffer_size)
                return NULL;
        }
 
-       CloseTransientFile(fd);
+       if (CloseTransientFile(fd))
+               ereport(LOG,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m", PGSS_TEXT_FILE)));
 
        *buffer_size = stat.st_size;
        return buf;

diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index f5cf9ffc9c751d4a99a39d410d5e1d956c3a7e46..bce4274362ca6955746a8a42d1b769ec369ce141 100644 (file)
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -1202,7 +1202,10 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
                                 errmsg("could not fsync file \"%s\": %m", path)));
        pgstat_report_wait_end();
 
-       CloseTransientFile(fd);
+       if (CloseTransientFile(fd))
+               ereport(ERROR,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m", path)));
 }
 
 /* ---
@@ -1300,7 +1303,11 @@ CheckPointLogicalRewriteHeap(void)
                                                (errcode_for_file_access(),
                                                 errmsg("could not fsync file \"%s\": %m", path)));
                        pgstat_report_wait_end();
-                       CloseTransientFile(fd);
+
+                       if (CloseTransientFile(fd))
+                               ereport(ERROR,
+                                               (errcode_for_file_access(),
+                                                errmsg("could not close file \"%s\": %m", path)));
                }
        }
        FreeDir(mappings_dir);

diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 3623352b9c6b290611cf5d70ab1fac3a3394bfb1..974d42fc866fa88e6a5d2421bccbb9adec42d45b 100644 (file)
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -599,7 +599,7 @@ SimpleLruDoesPhysicalPageExist(SlruCtl ctl, int pageno)
 
        SlruFileName(ctl, path, segno);
 
-       fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
+       fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
        if (fd < 0)
        {
                /* expected: file doesn't exist */
@@ -621,7 +621,13 @@ SimpleLruDoesPhysicalPageExist(SlruCtl ctl, int pageno)
 
        result = endpos >= (off_t) (offset + BLCKSZ);
 
-       CloseTransientFile(fd);
+       if (CloseTransientFile(fd))
+       {
+               slru_errcause = SLRU_CLOSE_FAILED;
+               slru_errno = errno;
+               return false;
+       }
+
        return result;
 }
 
@@ -654,7 +660,7 @@ SlruPhysicalReadPage(SlruCtl ctl, int pageno, int slotno)
         * SlruPhysicalWritePage).  Hence, if we are InRecovery, allow the case
         * where the file doesn't exist, and return zeroes instead.
         */
-       fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
+       fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
        if (fd < 0)
        {
                if (errno != ENOENT || !InRecovery)

diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/transam/timeline.c
index c96c8b60babf3dbc568b55249b8e914afdc7a749..cbd9b2cee19b74bd4a884165e08395316e39c4c5 100644 (file)
--- a/src/backend/access/transam/timeline.c
+++ b/src/backend/access/transam/timeline.c
@@ -370,7 +370,11 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
                        }
                        pgstat_report_wait_end();
                }
-               CloseTransientFile(srcfd);
+
+               if (CloseTransientFile(srcfd))
+                       ereport(ERROR,
+                                       (errcode_for_file_access(),
+                                        errmsg("could not close file \"%s\": %m", path)));
        }
 
        /*
@@ -416,7 +420,6 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
                                (errcode_for_file_access(),
                                 errmsg("could not close file \"%s\": %m", tmppath)));
 
-
        /*
         * Now move the completed history file into place with its final name.
         */
@@ -495,7 +498,6 @@ writeTimeLineHistoryFile(TimeLineID tli, char *content, int size)
                                (errcode_for_file_access(),
                                 errmsg("could not close file \"%s\": %m", tmppath)));
 
-
        /*
         * Now move the completed history file into place with its final name.
         */

diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 64679dd2de92c0281f75fa657aa5f0f80319ab56..21986e48fe2fe3a308c9d498bd9d85bb62f983eb 100644 (file)
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1297,7 +1297,11 @@ ReadTwoPhaseFile(TransactionId xid, bool missing_ok)
        }
 
        pgstat_report_wait_end();
-       CloseTransientFile(fd);
+
+       if (CloseTransientFile(fd))
+               ereport(ERROR,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m", path)));
 
        hdr = (TwoPhaseFileHeader *) buf;
        if (hdr->magic != TWOPHASE_MAGIC)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 0fdd82a287f4f6111ec0339b6f870f3124b66ff8..c7047738b6739f0433e087138a84edf878a6531d 100644 (file)
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3469,7 +3469,10 @@ XLogFileCopy(XLogSegNo destsegno, TimeLineID srcTLI, XLogSegNo srcsegno,
                                (errcode_for_file_access(),
                                 errmsg("could not close file \"%s\": %m", tmppath)));
 
-       CloseTransientFile(srcfd);
+       if (CloseTransientFile(srcfd))
+               ereport(ERROR,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m", path)));
 
        /*
         * Now move the segment into place with its final name.

diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index 521236d7c970c0bb7b2458b0b1efe56713089208..68f83a9bfd887b9ada4baaf465681ad08a11a9b7 100644 (file)
--- a/src/backend/libpq/be-fsstubs.c
+++ b/src/backend/libpq/be-fsstubs.c
@@ -455,7 +455,12 @@ lo_import_internal(text *filename, Oid lobjOid)
                                                fnamebuf)));
 
        inv_close(lobj);
-       CloseTransientFile(fd);
+
+       if (CloseTransientFile(fd))
+               ereport(ERROR,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m",
+                                               fnamebuf)));
 
        return oid;
 }
@@ -524,7 +529,12 @@ be_lo_export(PG_FUNCTION_ARGS)
                                                        fnamebuf)));
        }
 
-       CloseTransientFile(fd);
+       if (CloseTransientFile(fd))
+               ereport(ERROR,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m",
+                                               fnamebuf)));
+
        inv_close(lobj);
 
        PG_RETURN_INT32(1);

diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index dad2b3d065a02433446dc29073fa0dbe80cb7838..42fd8f5b6b7cd4584ebac98e1fcaa82fb4112a32 100644 (file)
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -658,7 +658,11 @@ CheckPointReplicationOrigin(void)
                                                tmppath)));
        }
 
-       CloseTransientFile(tmpfd);
+       if (CloseTransientFile(tmpfd))
+               ereport(PANIC,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m",
+                                               tmppath)));
 
        /* fsync, rename to permanent file, fsync file and directory */
        durable_rename(tmppath, path, PANIC);
@@ -793,7 +797,11 @@ StartupReplicationOrigin(void)
                                 errmsg("replication slot checkpoint has wrong checksum %u, expected %u",
                                                crc, file_crc)));
 
-       CloseTransientFile(fd);
+       if (CloseTransientFile(fd))
+               ereport(PANIC,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m",
+                                               path)));
 }
 
 void

diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 2b486b5e9f3afdfd243351a16205008608c09e62..2cfdf1c9ac9a6a7ff43d4e3a1b6a846944cbe973 100644 (file)
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -3360,7 +3360,10 @@ ApplyLogicalMappingFile(HTAB *tuplecid_data, Oid relid, const char *fname)
                }
        }
 
-       CloseTransientFile(fd);
+       if (CloseTransientFile(fd))
+               ereport(ERROR,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m", path)));
 }
 
 

diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index a59896f0825424a91969d08ec13af1bb1f0495f0..3e9d4cd79f95622cc8e3eaedfba7fa37271edce4 100644 (file)
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1651,7 +1651,11 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn)
                                 errmsg("could not fsync file \"%s\": %m", tmppath)));
        }
        pgstat_report_wait_end();
-       CloseTransientFile(fd);
+
+       if (CloseTransientFile(fd))
+               ereport(ERROR,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m", tmppath)));
 
        fsync_fname("pg_logical/snapshots", true);
 
@@ -1846,7 +1850,10 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
        }
        COMP_CRC32C(checksum, ondisk.builder.committed.xip, sz);
 
-       CloseTransientFile(fd);
+       if (CloseTransientFile(fd))
+               ereport(ERROR,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m", path)));
 
        FIN_CRC32C(checksum);
 

diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 33b23b6b6d21e2ebc89fe7aff726ca887a00f39c..006446b663ee68d51323c81f191d7984a6442a89 100644 (file)
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1315,7 +1315,11 @@ SaveSlotToPath(ReplicationSlot *slot, const char *dir, int elevel)
        }
        pgstat_report_wait_end();
 
-       CloseTransientFile(fd);
+       if (CloseTransientFile(fd))
+               ereport(elevel,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m",
+                                               tmppath)));
 
        /* rename to permanent file, fsync file and directory */
        if (rename(tmppath, path) != 0)
@@ -1377,7 +1381,7 @@ RestoreSlotFromDisk(const char *name)
 
        elog(DEBUG1, "restoring replication slot from \"%s\"", path);
 
-       fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
+       fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
 
        /*
         * We do not need to handle this as we are rename()ing the directory into
@@ -1477,7 +1481,10 @@ RestoreSlotFromDisk(const char *name)
                                                        path, readBytes, (Size) cp.length)));
        }
 
-       CloseTransientFile(fd);
+       if (CloseTransientFile(fd))
+               ereport(PANIC,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m", path)));
 
        /* now verify the CRC */
        INIT_CRC32C(checksum);

diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 9b143f361b8783ef24d2030e522124e85ff55fc4..4bb98ef352a41b4e6005a3cfc82cccdb0e3e3fa6 100644 (file)
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -521,7 +521,11 @@ SendTimeLineHistory(TimeLineHistoryCmd *cmd)
                pq_sendbytes(&buf, rbuf.data, nread);
                bytesleft -= nread;
        }
-       CloseTransientFile(fd);
+
+       if (CloseTransientFile(fd))
+               ereport(ERROR,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m", path)));
 
        pq_endmessage(&buf);
 }

diff --git a/src/backend/storage/file/copydir.c b/src/backend/storage/file/copydir.c
index 1f766d20d19d4ed222a92a3e3c2ae273b28dd6b9..342d078a8ff98818e0e342c4fa16b4c08e4cbca0 100644 (file)
--- a/src/backend/storage/file/copydir.c
+++ b/src/backend/storage/file/copydir.c
@@ -218,7 +218,10 @@ copy_file(char *fromfile, char *tofile)
                                (errcode_for_file_access(),
                                 errmsg("could not close file \"%s\": %m", tofile)));
 
-       CloseTransientFile(srcfd);
+       if (CloseTransientFile(srcfd))
+               ereport(ERROR,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m", fromfile)));
 
        pfree(buffer);
 }

diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 1ba0ddac107b27a13dccd786df90d54c964393d7..fdac9850e02371b06449b592a6cc80c3750746e5 100644 (file)
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -646,7 +646,14 @@ durable_rename(const char *oldfile, const char *newfile, int elevel)
                                         errmsg("could not fsync file \"%s\": %m", newfile)));
                        return -1;
                }
-               CloseTransientFile(fd);
+
+               if (CloseTransientFile(fd))
+               {
+                       ereport(elevel,
+                                       (errcode_for_file_access(),
+                                        errmsg("could not close file \"%s\": %m", newfile)));
+                       return -1;
+               }
        }
 
        /* Time to do the real deal... */
@@ -3295,7 +3302,10 @@ pre_sync_fname(const char *fname, bool isdir, int elevel)
         */
        pg_flush_data(fd, 0, 0);
 
-       (void) CloseTransientFile(fd);
+       if (CloseTransientFile(fd))
+               ereport(elevel,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m", fname)));
 }
 
 #endif                                                 /* PG_FLUSH_DATA_WORKS */
@@ -3394,7 +3404,13 @@ fsync_fname_ext(const char *fname, bool isdir, bool ignore_perm, int elevel)
                return -1;
        }
 
-       (void) CloseTransientFile(fd);
+       if (CloseTransientFile(fd))
+       {
+               ereport(elevel,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m", fname)));
+               return -1;
+       }
 
        return 0;
 }

diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index aeda32c9c59176496f5d2b348df22548f3a44905..a22c7928e74b084b04baeb67eed838c80e6be71b 100644 (file)
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -916,7 +916,15 @@ dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
        }
        *mapped_address = address;
        *mapped_size = request_size;
-       CloseTransientFile(fd);
+
+       if (CloseTransientFile(fd))
+       {
+               ereport(elevel,
+                               (errcode_for_file_access(),
+                                errmsg("could not close shared memory segment \"%s\": %m",
+                                               name)));
+               return false;
+       }
 
        return true;
 }

diff --git a/src/backend/utils/cache/relmapper.c b/src/backend/utils/cache/relmapper.c
index 5e61d908fdbb34d9f6163cdb78837999080b372d..f870a07d2a1927e1dcbf86a26fa8ded69b15da10 100644 (file)
--- a/src/backend/utils/cache/relmapper.c
+++ b/src/backend/utils/cache/relmapper.c
@@ -747,7 +747,11 @@ load_relmap_file(bool shared)
        }
        pgstat_report_wait_end();
 
-       CloseTransientFile(fd);
+       if (CloseTransientFile(fd))
+               ereport(FATAL,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m",
+                                               mapfilename)));
 
        /* check for correct magic number, etc */
        if (map->magic != RELMAPPER_FILEMAGIC ||

diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c
index 000f3c66d626e31a8d043e5a5b8774e0fc333d5b..6289a4343adba12f5377d4d91bea3d28ac1f464e 100644 (file)
--- a/src/common/controldata_utils.c
+++ b/src/common/controldata_utils.c
@@ -100,9 +100,18 @@ get_controlfile(const char *DataDir, const char *progname, bool *crc_ok_p)
        }
 
 #ifndef FRONTEND
-       CloseTransientFile(fd);
+       if (CloseTransientFile(fd))
+               ereport(ERROR,
+                               (errcode_for_file_access(),
+                                errmsg("could not close file \"%s\": %m",
+                                               ControlFilePath)));
 #else
-       close(fd);
+       if (close(fd))
+       {
+               fprintf(stderr, _("%s: could not close file \"%s\": %s\n"),
+                               progname, ControlFilePath, strerror(errno));
+               exit(EXIT_FAILURE);
+       }
 #endif
 
        /* Check the CRC. */