Handle close() failures more robustly in pg_dump and pg_basebackup.

tglsfdc · tglsfdc · commit bbda88c3383d · 2021-11-17T13:08:25.000-05:00
Coverity complained that applying get_gz_error after a failed gzclose, as we did in one place in pg_basebackup, is unsafe. I think it's right: it's entirely likely that the call is touching freed memory. Change that to inspect errno, as we do for other gzclose calls. Also, be careful to initialize errno to zero immediately before any gzclose() call where we care about the error status. (There are some calls where we don't, because we already failed at some previous step.) This ensures that we don't get a misleadingly irrelevant error code if gzclose() fails in a way that doesn't set errno. We could work harder at that, but it looks to me like all such cases are basically can't-happen if we're not misusing zlib, so it's not worth the extra notational cruft that would be required. Also, fix several places that simply failed to check for close-time errors at all, mostly at some remove from the close or gzclose itself; and one place that did check but didn't bother to report the errno. Back-patch to v12. These mistakes are older than that, but between the frontend logging API changes that happened in v12 and the fact that frontend code can't rely on %m before that, the patch would need substantial revision to work in older branches. It doesn't quite seem worth the trouble given the lack of related field complaints. Patch by me; thanks to Michael Paquier for review. Discussion: https://postgr.es/m/1343113.1636489231@sss.pgh.pa.us
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1262,10 +1262,11 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
 #ifdef HAVE_LIBZ
 	if (state.ztarfile != NULL)
 	{
+		errno = 0;				/* in case gzclose() doesn't set it */
 		if (gzclose(state.ztarfile) != 0)
 		{
-			pg_log_error("could not close compressed file \"%s\": %s",
-						 state.filename, get_gz_error(state.ztarfile));
+			pg_log_error("could not close compressed file \"%s\": %m",
+						 state.filename);
 			exit(1);
 		}
 	}
diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
@@ -71,7 +71,12 @@ mark_file_as_archived(StreamCtl *stream, const char *fname)
 		return false;
 	}
 
-	stream->walmethod->close(f, CLOSE_NORMAL);
+	if (stream->walmethod->close(f, CLOSE_NORMAL) != 0)
+	{
+		pg_log_error("could not close archive status file \"%s\": %s",
+					 tmppath, stream->walmethod->getlasterror());
+		return false;
+	}
 
 	return true;
 }
diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c
@@ -235,7 +235,10 @@ dir_close(Walfile f, WalCloseMethod method)
 
 #ifdef HAVE_LIBZ
 	if (dir_data->compression > 0)
+	{
+		errno = 0;				/* in case gzclose() doesn't set it */
 		r = gzclose(df->gzfp);
+	}
 	else
 #endif
 		r = close(df->fd);
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
@@ -268,6 +268,7 @@ CloseArchive(Archive *AHX)
 	AH->ClosePtr(AH);
 
 	/* Close the output */
+	errno = 0;					/* in case gzclose() doesn't set it */
 	if (AH->gzOut)
 		res = GZCLOSE(AH->OF);
 	else if (AH->OF != stdout)
@@ -1578,6 +1579,7 @@ RestoreOutput(ArchiveHandle *AH, OutputContext savedContext)
 {
 	int			res;
 
+	errno = 0;					/* in case gzclose() doesn't set it */
 	if (AH->gzOut)
 		res = GZCLOSE(AH->OF);
 	else
diff --git a/src/bin/pg_dump/pg_backup_directory.c b/src/bin/pg_dump/pg_backup_directory.c
@@ -369,7 +369,8 @@ _EndData(ArchiveHandle *AH, TocEntry *te)
 	lclContext *ctx = (lclContext *) AH->formatData;
 
 	/* Close the file */
-	cfclose(ctx->dataFH);
+	if (cfclose(ctx->dataFH) != 0)
+		fatal("could not close data file: %m");
 
 	ctx->dataFH = NULL;
 }
@@ -680,7 +681,8 @@ _EndBlob(ArchiveHandle *AH, TocEntry *te, Oid oid)
 	int			len;
 
 	/* Close the BLOB data file itself */
-	cfclose(ctx->dataFH);
+	if (cfclose(ctx->dataFH) != 0)
+		fatal("could not close blob data file: %m");
 	ctx->dataFH = NULL;
 
 	/* register the blob in blobs.toc */
@@ -699,7 +701,8 @@ _EndBlobs(ArchiveHandle *AH, TocEntry *te)
 {
 	lclContext *ctx = (lclContext *) AH->formatData;
 
-	cfclose(ctx->blobsTocFH);
+	if (cfclose(ctx->blobsTocFH) != 0)
+		fatal("could not close blobs TOC file: %m");
 	ctx->blobsTocFH = NULL;
 }
 
diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c
@@ -438,8 +438,11 @@ tarClose(ArchiveHandle *AH, TAR_MEMBER *th)
 	 * Close the GZ file since we dup'd. This will flush the buffers.
 	 */
 	if (AH->compression != 0)
+	{
+		errno = 0;				/* in case gzclose() doesn't set it */
 		if (GZCLOSE(th->zFH) != 0)
-			fatal("could not close tar member");
+			fatal("could not close tar member: %m");
+	}
 
 	if (th->mode == 'w')
 		_tarAddFile(AH, th);	/* This will close the temp file */

Original file line number	Diff line number	Diff line change
`@@ -1262,10 +1262,11 @@ ReceiveTarFile(PGconn conn, PGresult res, int rownum)`
`1262`	`1262`	`#ifdef HAVE_LIBZ`
`1263`	`1263`	`if (state.ztarfile != NULL)`
`1264`	`1264`	`{`
	`1265`	`+ errno = 0; /* in case gzclose() doesn't set it */`
`1265`	`1266`	`if (gzclose(state.ztarfile) != 0)`
`1266`	`1267`	`{`
`1267`		`- pg_log_error("could not close compressed file \"%s\": %s",`
`1268`		`- state.filename, get_gz_error(state.ztarfile));`
	`1268`	`+ pg_log_error("could not close compressed file \"%s\": %m",`
	`1269`	`+ state.filename);`
`1269`	`1270`	`exit(1);`
`1270`	`1271`	`}`
`1271`	`1272`	`}`
Original file line number	Diff line number	Diff line change
`@@ -71,7 +71,12 @@ mark_file_as_archived(StreamCtl stream, const char fname)`
`71`	`71`	`return false;`
`72`	`72`	`}`
`73`	`73`
`74`		`- stream->walmethod->close(f, CLOSE_NORMAL);`
	`74`	`+ if (stream->walmethod->close(f, CLOSE_NORMAL) != 0)`
	`75`	`+ {`
	`76`	`+ pg_log_error("could not close archive status file \"%s\": %s",`
	`77`	`+ tmppath, stream->walmethod->getlasterror());`
	`78`	`+ return false;`
	`79`	`+ }`
`75`	`80`
`76`	`81`	`return true;`
`77`	`82`	`}`