Skip to content

Commit 82a5649

Browse files
michaelpqmichaelpq
committed
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
1 parent 2e616de commit 82a5649

File tree

17 files changed

+134
-30
lines changed

17 files changed

+134
-30
lines changed

‎contrib/pg_stat_statements/pg_stat_statements.c

+4-1
Original file line numberDiff line numberDiff line change
@@ -1991,7 +1991,10 @@ qtext_load_file(Size *buffer_size)
19911991
return NULL;
19921992
}
19931993

1994-
CloseTransientFile(fd);
1994+
if (CloseTransientFile(fd))
1995+
ereport(LOG,
1996+
(errcode_for_file_access(),
1997+
errmsg("could not close file \"%s\": %m", PGSS_TEXT_FILE)));
19951998

19961999
*buffer_size = stat.st_size;
19972000
return buf;

‎src/backend/access/heap/rewriteheap.c

+9-2
Original file line numberDiff line numberDiff line change
@@ -1202,7 +1202,10 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
12021202
errmsg("could not fsync file \"%s\": %m", path)));
12031203
pgstat_report_wait_end();
12041204

1205-
CloseTransientFile(fd);
1205+
if (CloseTransientFile(fd))
1206+
ereport(ERROR,
1207+
(errcode_for_file_access(),
1208+
errmsg("could not close file \"%s\": %m", path)));
12061209
}
12071210

12081211
/* ---
@@ -1300,7 +1303,11 @@ CheckPointLogicalRewriteHeap(void)
13001303
(errcode_for_file_access(),
13011304
errmsg("could not fsync file \"%s\": %m", path)));
13021305
pgstat_report_wait_end();
1303-
CloseTransientFile(fd);
1306+
1307+
if (CloseTransientFile(fd))
1308+
ereport(ERROR,
1309+
(errcode_for_file_access(),
1310+
errmsg("could not close file \"%s\": %m", path)));
13041311
}
13051312
}
13061313
FreeDir(mappings_dir);

‎src/backend/access/transam/slru.c

+9-3
Original file line numberDiff line numberDiff line change
@@ -599,7 +599,7 @@ SimpleLruDoesPhysicalPageExist(SlruCtl ctl, int pageno)
599599

600600
SlruFileName(ctl, path, segno);
601601

602-
fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
602+
fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
603603
if (fd < 0)
604604
{
605605
/* expected: file doesn't exist */
@@ -621,7 +621,13 @@ SimpleLruDoesPhysicalPageExist(SlruCtl ctl, int pageno)
621621

622622
result = endpos >= (off_t) (offset + BLCKSZ);
623623

624-
CloseTransientFile(fd);
624+
if (CloseTransientFile(fd))
625+
{
626+
slru_errcause = SLRU_CLOSE_FAILED;
627+
slru_errno = errno;
628+
return false;
629+
}
630+
625631
return result;
626632
}
627633

@@ -654,7 +660,7 @@ SlruPhysicalReadPage(SlruCtl ctl, int pageno, int slotno)
654660
* SlruPhysicalWritePage). Hence, if we are InRecovery, allow the case
655661
* where the file doesn't exist, and return zeroes instead.
656662
*/
657-
fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
663+
fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
658664
if (fd < 0)
659665
{
660666
if (errno != ENOENT || !InRecovery)

‎src/backend/access/transam/timeline.c

+5-3
Original file line numberDiff line numberDiff line change
@@ -370,7 +370,11 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
370370
}
371371
pgstat_report_wait_end();
372372
}
373-
CloseTransientFile(srcfd);
373+
374+
if (CloseTransientFile(srcfd))
375+
ereport(ERROR,
376+
(errcode_for_file_access(),
377+
errmsg("could not close file \"%s\": %m", path)));
374378
}
375379

376380
/*
@@ -416,7 +420,6 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
416420
(errcode_for_file_access(),
417421
errmsg("could not close file \"%s\": %m", tmppath)));
418422

419-
420423
/*
421424
* Now move the completed history file into place with its final name.
422425
*/
@@ -495,7 +498,6 @@ writeTimeLineHistoryFile(TimeLineID tli, char *content, int size)
495498
(errcode_for_file_access(),
496499
errmsg("could not close file \"%s\": %m", tmppath)));
497500

498-
499501
/*
500502
* Now move the completed history file into place with its final name.
501503
*/

‎src/backend/access/transam/twophase.c

+5-1
Original file line numberDiff line numberDiff line change
@@ -1297,7 +1297,11 @@ ReadTwoPhaseFile(TransactionId xid, bool missing_ok)
12971297
}
12981298

12991299
pgstat_report_wait_end();
1300-
CloseTransientFile(fd);
1300+
1301+
if (CloseTransientFile(fd))
1302+
ereport(ERROR,
1303+
(errcode_for_file_access(),
1304+
errmsg("could not close file \"%s\": %m", path)));
13011305

13021306
hdr = (TwoPhaseFileHeader *) buf;
13031307
if (hdr->magic != TWOPHASE_MAGIC)

‎src/backend/access/transam/xlog.c

+4-1
Original file line numberDiff line numberDiff line change
@@ -3469,7 +3469,10 @@ XLogFileCopy(XLogSegNo destsegno, TimeLineID srcTLI, XLogSegNo srcsegno,
34693469
(errcode_for_file_access(),
34703470
errmsg("could not close file \"%s\": %m", tmppath)));
34713471

3472-
CloseTransientFile(srcfd);
3472+
if (CloseTransientFile(srcfd))
3473+
ereport(ERROR,
3474+
(errcode_for_file_access(),
3475+
errmsg("could not close file \"%s\": %m", path)));
34733476

34743477
/*
34753478
* Now move the segment into place with its final name.

‎src/backend/libpq/be-fsstubs.c

+12-2
Original file line numberDiff line numberDiff line change
@@ -455,7 +455,12 @@ lo_import_internal(text *filename, Oid lobjOid)
455455
fnamebuf)));
456456

457457
inv_close(lobj);
458-
CloseTransientFile(fd);
458+
459+
if (CloseTransientFile(fd))
460+
ereport(ERROR,
461+
(errcode_for_file_access(),
462+
errmsg("could not close file \"%s\": %m",
463+
fnamebuf)));
459464

460465
return oid;
461466
}
@@ -524,7 +529,12 @@ be_lo_export(PG_FUNCTION_ARGS)
524529
fnamebuf)));
525530
}
526531

527-
CloseTransientFile(fd);
532+
if (CloseTransientFile(fd))
533+
ereport(ERROR,
534+
(errcode_for_file_access(),
535+
errmsg("could not close file \"%s\": %m",
536+
fnamebuf)));
537+
528538
inv_close(lobj);
529539

530540
PG_RETURN_INT32(1);

‎src/backend/replication/logical/origin.c

+10-2
Original file line numberDiff line numberDiff line change
@@ -658,7 +658,11 @@ CheckPointReplicationOrigin(void)
658658
tmppath)));
659659
}
660660

661-
CloseTransientFile(tmpfd);
661+
if (CloseTransientFile(tmpfd))
662+
ereport(PANIC,
663+
(errcode_for_file_access(),
664+
errmsg("could not close file \"%s\": %m",
665+
tmppath)));
662666

663667
/* fsync, rename to permanent file, fsync file and directory */
664668
durable_rename(tmppath, path, PANIC);
@@ -793,7 +797,11 @@ StartupReplicationOrigin(void)
793797
errmsg("replication slot checkpoint has wrong checksum %u, expected %u",
794798
crc, file_crc)));
795799

796-
CloseTransientFile(fd);
800+
if (CloseTransientFile(fd))
801+
ereport(PANIC,
802+
(errcode_for_file_access(),
803+
errmsg("could not close file \"%s\": %m",
804+
path)));
797805
}
798806

799807
void

‎src/backend/replication/logical/reorderbuffer.c

+4-1
Original file line numberDiff line numberDiff line change
@@ -3360,7 +3360,10 @@ ApplyLogicalMappingFile(HTAB *tuplecid_data, Oid relid, const char *fname)
33603360
}
33613361
}
33623362

3363-
CloseTransientFile(fd);
3363+
if (CloseTransientFile(fd))
3364+
ereport(ERROR,
3365+
(errcode_for_file_access(),
3366+
errmsg("could not close file \"%s\": %m", path)));
33643367
}
33653368

33663369

‎src/backend/replication/logical/snapbuild.c

+9-2
Original file line numberDiff line numberDiff line change
@@ -1651,7 +1651,11 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn)
16511651
errmsg("could not fsync file \"%s\": %m", tmppath)));
16521652
}
16531653
pgstat_report_wait_end();
1654-
CloseTransientFile(fd);
1654+
1655+
if (CloseTransientFile(fd))
1656+
ereport(ERROR,
1657+
(errcode_for_file_access(),
1658+
errmsg("could not close file \"%s\": %m", tmppath)));
16551659

16561660
fsync_fname("pg_logical/snapshots", true);
16571661

@@ -1846,7 +1850,10 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
18461850
}
18471851
COMP_CRC32C(checksum, ondisk.builder.committed.xip, sz);
18481852

1849-
CloseTransientFile(fd);
1853+
if (CloseTransientFile(fd))
1854+
ereport(ERROR,
1855+
(errcode_for_file_access(),
1856+
errmsg("could not close file \"%s\": %m", path)));
18501857

18511858
FIN_CRC32C(checksum);
18521859

‎src/backend/replication/slot.c

+10-3
Original file line numberDiff line numberDiff line change
@@ -1315,7 +1315,11 @@ SaveSlotToPath(ReplicationSlot *slot, const char *dir, int elevel)
13151315
}
13161316
pgstat_report_wait_end();
13171317

1318-
CloseTransientFile(fd);
1318+
if (CloseTransientFile(fd))
1319+
ereport(elevel,
1320+
(errcode_for_file_access(),
1321+
errmsg("could not close file \"%s\": %m",
1322+
tmppath)));
13191323

13201324
/* rename to permanent file, fsync file and directory */
13211325
if (rename(tmppath, path) != 0)
@@ -1377,7 +1381,7 @@ RestoreSlotFromDisk(const char *name)
13771381

13781382
elog(DEBUG1, "restoring replication slot from \"%s\"", path);
13791383

1380-
fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
1384+
fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
13811385

13821386
/*
13831387
* We do not need to handle this as we are rename()ing the directory into
@@ -1477,7 +1481,10 @@ RestoreSlotFromDisk(const char *name)
14771481
path, readBytes, (Size) cp.length)));
14781482
}
14791483

1480-
CloseTransientFile(fd);
1484+
if (CloseTransientFile(fd))
1485+
ereport(PANIC,
1486+
(errcode_for_file_access(),
1487+
errmsg("could not close file \"%s\": %m", path)));
14811488

14821489
/* now verify the CRC */
14831490
INIT_CRC32C(checksum);

‎src/backend/replication/walsender.c

+5-1
Original file line numberDiff line numberDiff line change
@@ -521,7 +521,11 @@ SendTimeLineHistory(TimeLineHistoryCmd *cmd)
521521
pq_sendbytes(&buf, rbuf.data, nread);
522522
bytesleft -= nread;
523523
}
524-
CloseTransientFile(fd);
524+
525+
if (CloseTransientFile(fd))
526+
ereport(ERROR,
527+
(errcode_for_file_access(),
528+
errmsg("could not close file \"%s\": %m", path)));
525529

526530
pq_endmessage(&buf);
527531
}

‎src/backend/storage/file/copydir.c

+4-1
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,10 @@ copy_file(char *fromfile, char *tofile)
218218
(errcode_for_file_access(),
219219
errmsg("could not close file \"%s\": %m", tofile)));
220220

221-
CloseTransientFile(srcfd);
221+
if (CloseTransientFile(srcfd))
222+
ereport(ERROR,
223+
(errcode_for_file_access(),
224+
errmsg("could not close file \"%s\": %m", fromfile)));
222225

223226
pfree(buffer);
224227
}

‎src/backend/storage/file/fd.c

+19-3
Original file line numberDiff line numberDiff line change
@@ -646,7 +646,14 @@ durable_rename(const char *oldfile, const char *newfile, int elevel)
646646
errmsg("could not fsync file \"%s\": %m", newfile)));
647647
return -1;
648648
}
649-
CloseTransientFile(fd);
649+
650+
if (CloseTransientFile(fd))
651+
{
652+
ereport(elevel,
653+
(errcode_for_file_access(),
654+
errmsg("could not close file \"%s\": %m", newfile)));
655+
return -1;
656+
}
650657
}
651658

652659
/* Time to do the real deal... */
@@ -3295,7 +3302,10 @@ pre_sync_fname(const char *fname, bool isdir, int elevel)
32953302
*/
32963303
pg_flush_data(fd, 0, 0);
32973304

3298-
(void) CloseTransientFile(fd);
3305+
if (CloseTransientFile(fd))
3306+
ereport(elevel,
3307+
(errcode_for_file_access(),
3308+
errmsg("could not close file \"%s\": %m", fname)));
32993309
}
33003310

33013311
#endif /* PG_FLUSH_DATA_WORKS */
@@ -3394,7 +3404,13 @@ fsync_fname_ext(const char *fname, bool isdir, bool ignore_perm, int elevel)
33943404
return -1;
33953405
}
33963406

3397-
(void) CloseTransientFile(fd);
3407+
if (CloseTransientFile(fd))
3408+
{
3409+
ereport(elevel,
3410+
(errcode_for_file_access(),
3411+
errmsg("could not close file \"%s\": %m", fname)));
3412+
return -1;
3413+
}
33983414

33993415
return 0;
34003416
}

‎src/backend/storage/ipc/dsm_impl.c

+9-1
Original file line numberDiff line numberDiff line change
@@ -916,7 +916,15 @@ dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
916916
}
917917
*mapped_address = address;
918918
*mapped_size = request_size;
919-
CloseTransientFile(fd);
919+
920+
if (CloseTransientFile(fd))
921+
{
922+
ereport(elevel,
923+
(errcode_for_file_access(),
924+
errmsg("could not close shared memory segment \"%s\": %m",
925+
name)));
926+
return false;
927+
}
920928

921929
return true;
922930
}

‎src/backend/utils/cache/relmapper.c

+5-1
Original file line numberDiff line numberDiff line change
@@ -747,7 +747,11 @@ load_relmap_file(bool shared)
747747
}
748748
pgstat_report_wait_end();
749749

750-
CloseTransientFile(fd);
750+
if (CloseTransientFile(fd))
751+
ereport(FATAL,
752+
(errcode_for_file_access(),
753+
errmsg("could not close file \"%s\": %m",
754+
mapfilename)));
751755

752756
/* check for correct magic number, etc */
753757
if (map->magic != RELMAPPER_FILEMAGIC ||

‎src/common/controldata_utils.c

+11-2
Original file line numberDiff line numberDiff line change
@@ -100,9 +100,18 @@ get_controlfile(const char *DataDir, const char *progname, bool *crc_ok_p)
100100
}
101101

102102
#ifndef FRONTEND
103-
CloseTransientFile(fd);
103+
if (CloseTransientFile(fd))
104+
ereport(ERROR,
105+
(errcode_for_file_access(),
106+
errmsg("could not close file \"%s\": %m",
107+
ControlFilePath)));
104108
#else
105-
close(fd);
109+
if (close(fd))
110+
{
111+
fprintf(stderr, _("%s: could not close file \"%s\": %s\n"),
112+
progname, ControlFilePath, strerror(errno));
113+
exit(EXIT_FAILURE);
114+
}
106115
#endif
107116

108117
/* Check the CRC. */

0 commit comments

Comments
 (0)