From: Michael Paquier Date: Fri, 8 Mar 2019 23:50:55 +0000 (+0900) Subject: Tighten use of OpenTransientFile and CloseTransientFile X-Git-Tag: REL_12_BETA1~590 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=82a5649fb9dbef12d04cd24799be6bf298d889a6;p=postgresql 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 9905593661..7b39283c89 100644 --- 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 f5cf9ffc9c..bce4274362 100644 --- 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 3623352b9c..974d42fc86 100644 --- 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 c96c8b60ba..cbd9b2cee1 100644 --- 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 64679dd2de..21986e48fe 100644 --- 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 0fdd82a287..c7047738b6 100644 --- 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 521236d7c9..68f83a9bfd 100644 --- 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 dad2b3d065..42fd8f5b6b 100644 --- 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 2b486b5e9f..2cfdf1c9ac 100644 --- 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 a59896f082..3e9d4cd79f 100644 --- 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 33b23b6b6d..006446b663 100644 --- 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 9b143f361b..4bb98ef352 100644 --- 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 1f766d20d1..342d078a8f 100644 --- 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 1ba0ddac10..fdac9850e0 100644 --- 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 aeda32c9c5..a22c7928e7 100644 --- 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 5e61d908fd..f870a07d2a 100644 --- 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 000f3c66d6..6289a4343a 100644 --- 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. */