From 6cb3372411fd6ed8ba0f8d36ae578a2daa517c16 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Mon, 25 Jun 2018 11:19:05 +0900 Subject: [PATCH] Address set of issues with errno handling System calls mixed up in error code paths are causing two issues which several code paths have not correctly handled: 1) For write() calls, sometimes the system may return less bytes than what has been written without errno being set. Some paths were careful enough to consider that case, and assumed that errno should be set to ENOSPC, other calls missed that. 2) errno generated by a system call is overwritten by other system calls which may succeed once an error code path is taken, causing what is reported to the user to be incorrect. This patch uses the brute-force approach of correcting all those code paths. Some refactoring could happen in the future, but this is let as future work, which is not targeted for back-branches anyway. Author: Michael Paquier Reviewed-by: Ashutosh Sharma Discussion: https://postgr.es/m/20180622061535.GD5215@paquier.xyz --- src/backend/access/heap/rewriteheap.c | 5 +++ src/backend/access/transam/twophase.c | 23 +++++++++++++ src/backend/access/transam/xlog.c | 7 ++++ src/backend/access/transam/xlogutils.c | 6 ++-- src/backend/replication/basebackup.c | 3 ++ src/backend/replication/logical/origin.c | 15 ++++++++ .../replication/logical/reorderbuffer.c | 4 ++- src/backend/replication/logical/snapbuild.c | 20 +++++++++++ src/backend/replication/slot.c | 7 +++- src/bin/pg_basebackup/walmethods.c | 34 +++++++++++++++++-- 10 files changed, 118 insertions(+), 6 deletions(-) diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c index 8d3c861a33..ed7ba181c7 100644 --- a/src/backend/access/heap/rewriteheap.c +++ b/src/backend/access/heap/rewriteheap.c @@ -1168,9 +1168,14 @@ heap_xlog_logical_rewrite(XLogReaderState *r) /* write out tail end of mapping file (again) */ pgstat_report_wait_start(WAIT_EVENT_LOGICAL_REWRITE_MAPPING_WRITE); if (write(fd, data, len) != len) + { + /* if write didn't set errno, assume problem is no disk space */ + if (errno == 0) + errno = ENOSPC; ereport(ERROR, (errcode_for_file_access(), errmsg("could not write to file \"%s\": %m", path))); + } pgstat_report_wait_end(); /* diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c index 65194db70e..a9ef1b3d73 100644 --- a/src/backend/access/transam/twophase.c +++ b/src/backend/access/transam/twophase.c @@ -1241,12 +1241,17 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings) */ if (fstat(fd, &stat)) { + int save_errno = errno; + CloseTransientFile(fd); if (give_warnings) + { + errno = save_errno; ereport(WARNING, (errcode_for_file_access(), errmsg("could not stat two-phase state file \"%s\": %m", path))); + } return NULL; } @@ -1274,13 +1279,18 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings) pgstat_report_wait_start(WAIT_EVENT_TWOPHASE_FILE_READ); if (read(fd, buf, stat.st_size) != stat.st_size) { + int save_errno = errno; + pgstat_report_wait_end(); CloseTransientFile(fd); if (give_warnings) + { + errno = save_errno; ereport(WARNING, (errcode_for_file_access(), errmsg("could not read two-phase state file \"%s\": %m", path))); + } pfree(buf); return NULL; } @@ -1663,16 +1673,26 @@ RecreateTwoPhaseFile(TransactionId xid, void *content, int len) pgstat_report_wait_start(WAIT_EVENT_TWOPHASE_FILE_WRITE); if (write(fd, content, len) != len) { + int save_errno = errno; + pgstat_report_wait_end(); CloseTransientFile(fd); + + /* if write didn't set errno, assume problem is no disk space */ + errno = save_errno ? save_errno : ENOSPC; ereport(ERROR, (errcode_for_file_access(), errmsg("could not write two-phase state file: %m"))); } if (write(fd, &statefile_crc, sizeof(pg_crc32c)) != sizeof(pg_crc32c)) { + int save_errno = errno; + pgstat_report_wait_end(); CloseTransientFile(fd); + + /* if write didn't set errno, assume problem is no disk space */ + errno = save_errno ? save_errno : ENOSPC; ereport(ERROR, (errcode_for_file_access(), errmsg("could not write two-phase state file: %m"))); @@ -1686,7 +1706,10 @@ RecreateTwoPhaseFile(TransactionId xid, void *content, int len) pgstat_report_wait_start(WAIT_EVENT_TWOPHASE_FILE_SYNC); if (pg_fsync(fd) != 0) { + int save_errno = errno; + CloseTransientFile(fd); + errno = save_errno; ereport(ERROR, (errcode_for_file_access(), errmsg("could not fsync two-phase state file: %m"))); diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index adbd6a2126..1a419aa49b 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -3268,7 +3268,10 @@ XLogFileInit(XLogSegNo logsegno, bool *use_existent, bool use_lock) pgstat_report_wait_start(WAIT_EVENT_WAL_INIT_SYNC); if (pg_fsync(fd) != 0) { + int save_errno = errno; + close(fd); + errno = save_errno; ereport(ERROR, (errcode_for_file_access(), errmsg("could not fsync file \"%s\": %m", tmppath))); @@ -11675,8 +11678,10 @@ retry: if (lseek(readFile, (off_t) readOff, SEEK_SET) < 0) { char fname[MAXFNAMELEN]; + int save_errno = errno; XLogFileName(fname, curFileTLI, readSegNo, wal_segment_size); + errno = save_errno; ereport(emode_for_corrupt_record(emode, targetPagePtr + reqLen), (errcode_for_file_access(), errmsg("could not seek in log segment %s to offset %u: %m", @@ -11688,9 +11693,11 @@ retry: if (read(readFile, readBuf, XLOG_BLCKSZ) != XLOG_BLCKSZ) { char fname[MAXFNAMELEN]; + int save_errno = errno; pgstat_report_wait_end(); XLogFileName(fname, curFileTLI, readSegNo, wal_segment_size); + errno = save_errno; ereport(emode_for_corrupt_record(emode, targetPagePtr + reqLen), (errcode_for_file_access(), errmsg("could not read from log segment %s, offset %u: %m", diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c index 52fe55e2af..4ecdc9220f 100644 --- a/src/backend/access/transam/xlogutils.c +++ b/src/backend/access/transam/xlogutils.c @@ -718,9 +718,10 @@ XLogRead(char *buf, int segsize, TimeLineID tli, XLogRecPtr startptr, if (lseek(sendFile, (off_t) startoff, SEEK_SET) < 0) { char path[MAXPGPATH]; + int save_errno = errno; XLogFilePath(path, tli, sendSegNo, segsize); - + errno = save_errno; ereport(ERROR, (errcode_for_file_access(), errmsg("could not seek in log segment %s to offset %u: %m", @@ -741,9 +742,10 @@ XLogRead(char *buf, int segsize, TimeLineID tli, XLogRecPtr startptr, if (readbytes <= 0) { char path[MAXPGPATH]; + int save_errno = errno; XLogFilePath(path, tli, sendSegNo, segsize); - + errno = save_errno; ereport(ERROR, (errcode_for_file_access(), errmsg("could not read from log segment %s, offset %u, length %lu: %m", diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c index 5688cbe2e9..3f1eae38a9 100644 --- a/src/backend/replication/basebackup.c +++ b/src/backend/replication/basebackup.c @@ -495,6 +495,8 @@ perform_base_backup(basebackup_options *opt) fp = AllocateFile(pathbuf, "rb"); if (fp == NULL) { + int save_errno = errno; + /* * Most likely reason for this is that the file was already * removed by a checkpoint, so check for that to get a better @@ -502,6 +504,7 @@ perform_base_backup(basebackup_options *opt) */ CheckXLogRemoved(segno, tli); + errno = save_errno; ereport(ERROR, (errcode_for_file_access(), errmsg("could not open file \"%s\": %m", pathbuf))); diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c index 963878a5d8..3d3f6dff1b 100644 --- a/src/backend/replication/logical/origin.c +++ b/src/backend/replication/logical/origin.c @@ -578,7 +578,12 @@ CheckPointReplicationOrigin(void) /* write magic */ if ((write(tmpfd, &magic, sizeof(magic))) != sizeof(magic)) { + int save_errno = errno; + CloseTransientFile(tmpfd); + + /* if write didn't set errno, assume problem is no disk space */ + errno = save_errno ? save_errno : ENOSPC; ereport(PANIC, (errcode_for_file_access(), errmsg("could not write to file \"%s\": %m", @@ -617,7 +622,12 @@ CheckPointReplicationOrigin(void) if ((write(tmpfd, &disk_state, sizeof(disk_state))) != sizeof(disk_state)) { + int save_errno = errno; + CloseTransientFile(tmpfd); + + /* if write didn't set errno, assume problem is no disk space */ + errno = save_errno ? save_errno : ENOSPC; ereport(PANIC, (errcode_for_file_access(), errmsg("could not write to file \"%s\": %m", @@ -633,7 +643,12 @@ CheckPointReplicationOrigin(void) FIN_CRC32C(crc); if ((write(tmpfd, &crc, sizeof(crc))) != sizeof(crc)) { + int save_errno = errno; + CloseTransientFile(tmpfd); + + /* if write didn't set errno, assume problem is no disk space */ + errno = save_errno ? save_errno : ENOSPC; ereport(PANIC, (errcode_for_file_access(), errmsg("could not write to file \"%s\": %m", diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index e2f59bf580..3799ad4011 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -2304,7 +2304,9 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn, int save_errno = errno; CloseTransientFile(fd); - errno = save_errno; + + /* if write didn't set errno, assume problem is no disk space */ + errno = save_errno ? save_errno : ENOSPC; ereport(ERROR, (errcode_for_file_access(), errmsg("could not write to data file for XID %u: %m", diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c index 4123cdebcf..2c4a1bab4b 100644 --- a/src/backend/replication/logical/snapbuild.c +++ b/src/backend/replication/logical/snapbuild.c @@ -1605,7 +1605,12 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn) pgstat_report_wait_start(WAIT_EVENT_SNAPBUILD_WRITE); if ((write(fd, ondisk, needed_length)) != needed_length) { + int save_errno = errno; + CloseTransientFile(fd); + + /* if write didn't set errno, assume problem is no disk space */ + errno = save_errno ? save_errno : ENOSPC; ereport(ERROR, (errcode_for_file_access(), errmsg("could not write to file \"%s\": %m", tmppath))); @@ -1623,7 +1628,10 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn) pgstat_report_wait_start(WAIT_EVENT_SNAPBUILD_SYNC); if (pg_fsync(fd) != 0) { + int save_errno = errno; + CloseTransientFile(fd); + errno = save_errno; ereport(ERROR, (errcode_for_file_access(), errmsg("could not fsync file \"%s\": %m", tmppath))); @@ -1708,7 +1716,10 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn) pgstat_report_wait_end(); if (readBytes != SnapBuildOnDiskConstantSize) { + int save_errno = errno; + CloseTransientFile(fd); + errno = save_errno; ereport(ERROR, (errcode_for_file_access(), errmsg("could not read file \"%s\", read %d of %d: %m", @@ -1736,7 +1747,10 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn) pgstat_report_wait_end(); if (readBytes != sizeof(SnapBuild)) { + int save_errno = errno; + CloseTransientFile(fd); + errno = save_errno; ereport(ERROR, (errcode_for_file_access(), errmsg("could not read file \"%s\", read %d of %d: %m", @@ -1753,7 +1767,10 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn) pgstat_report_wait_end(); if (readBytes != sz) { + int save_errno = errno; + CloseTransientFile(fd); + errno = save_errno; ereport(ERROR, (errcode_for_file_access(), errmsg("could not read file \"%s\", read %d of %d: %m", @@ -1769,7 +1786,10 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn) pgstat_report_wait_end(); if (readBytes != sz) { + int save_errno = errno; + CloseTransientFile(fd); + errno = save_errno; ereport(ERROR, (errcode_for_file_access(), errmsg("could not read file \"%s\", read %d of %d: %m", diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index e8b76b2f20..f5927b4d1d 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -1274,7 +1274,9 @@ SaveSlotToPath(ReplicationSlot *slot, const char *dir, int elevel) pgstat_report_wait_end(); CloseTransientFile(fd); - errno = save_errno; + + /* if write didn't set errno, assume problem is no disk space */ + errno = save_errno ? save_errno : ENOSPC; ereport(elevel, (errcode_for_file_access(), errmsg("could not write to file \"%s\": %m", @@ -1378,7 +1380,10 @@ RestoreSlotFromDisk(const char *name) pgstat_report_wait_start(WAIT_EVENT_REPLICATION_SLOT_RESTORE_SYNC); if (pg_fsync(fd) != 0) { + int save_errno = errno; + CloseTransientFile(fd); + errno = save_errno; ereport(PANIC, (errcode_for_file_access(), errmsg("could not fsync file \"%s\": %m", diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c index 267a40debb..331d0e7275 100644 --- a/src/bin/pg_basebackup/walmethods.c +++ b/src/bin/pg_basebackup/walmethods.c @@ -128,7 +128,11 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_ pg_free(zerobuf); close(fd); - errno = save_errno; + + /* + * If write didn't set errno, assume problem is no disk space. + */ + errno = save_errno ? save_errno : ENOSPC; return NULL; } } @@ -442,7 +446,14 @@ tar_write_compressed_data(void *buf, size_t count, bool flush) size_t len = ZLIB_OUT_SIZE - tar_data->zp->avail_out; if (write(tar_data->fd, tar_data->zlibOut, len) != len) + { + /* + * If write didn't set errno, assume problem is no disk space. + */ + if (errno == 0) + errno = ENOSPC; return false; + } tar_data->zp->next_out = tar_data->zlibOut; tar_data->zp->avail_out = ZLIB_OUT_SIZE; @@ -623,7 +634,8 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_ save_errno = errno; pg_free(tar_data->currentfile); tar_data->currentfile = NULL; - errno = save_errno; + /* if write didn't set errno, assume problem is no disk space */ + errno = save_errno ? save_errno : ENOSPC; return NULL; } } @@ -818,7 +830,12 @@ tar_close(Walfile f, WalCloseMethod method) if (!tar_data->compression) { if (write(tar_data->fd, tf->header, 512) != 512) + { + /* if write didn't set errno, assume problem is no disk space */ + if (errno == 0) + errno = ENOSPC; return -1; + } } #ifdef HAVE_LIBZ else @@ -884,7 +901,12 @@ tar_finish(void) if (!tar_data->compression) { if (write(tar_data->fd, zerobuf, sizeof(zerobuf)) != sizeof(zerobuf)) + { + /* if write didn't set errno, assume problem is no disk space */ + if (errno == 0) + errno = ENOSPC; return false; + } } #ifdef HAVE_LIBZ else @@ -911,7 +933,15 @@ tar_finish(void) size_t len = ZLIB_OUT_SIZE - tar_data->zp->avail_out; if (write(tar_data->fd, tar_data->zlibOut, len) != len) + { + /* + * If write didn't set errno, assume problem is no disk + * space. + */ + if (errno == 0) + errno = ENOSPC; return false; + } } if (r == Z_STREAM_END) break;