From 211d49090a0f2f9579140653eca632dac1f9d5c4 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Fri, 5 Jan 2024 09:09:34 -0500 Subject: [PATCH v1 4/4] Repair various defects in dc212340058b4e7ecfc5a7a81ec50e7a207bf288. pg_combinebackup had various problems: * strncpy was used in various places where should be used instead, to avoid any possibility of the result not being \0-terminated. * scan_for_existing_tablespaces() failed to close the directory, and an error when opening the directory was reported with the wrong pathname. * write_reconstructed_file() contained some redundant and therefore dead code. * flush_manifest() didn't check the result of pg_checksum_update() as we do in other places, and misused a local pathname variable that shouldn't exist at all. In pg_basebackup, the wrong variable name was used in one place, due to a copy and paste that was not properly adjusted. Per Coverity and subsequent code inspection. --- src/bin/pg_basebackup/pg_basebackup.c | 2 +- src/bin/pg_combinebackup/pg_combinebackup.c | 15 +++++++++------ src/bin/pg_combinebackup/reconstruct.c | 5 ++--- src/bin/pg_combinebackup/write_manifest.c | 10 +++++----- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c index b734cce5d4..3b3cc242e0 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -709,7 +709,7 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier, PQserverVersion(conn) < MINIMUM_VERSION_FOR_PG_WAL ? "pg_xlog" : "pg_wal"); - if (pg_mkdir_p(statusdir, pg_dir_create_mode) != 0 && + if (pg_mkdir_p(summarydir, pg_dir_create_mode) != 0 && errno != EEXIST) pg_fatal("could not create directory \"%s\": %m", summarydir); } diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c index 6027e241f3..31ead7f405 100644 --- a/src/bin/pg_combinebackup/pg_combinebackup.c +++ b/src/bin/pg_combinebackup/pg_combinebackup.c @@ -773,8 +773,8 @@ process_directory_recursively(Oid tsoid, */ if (relative_path == NULL) { - strncpy(ifulldir, input_directory, MAXPGPATH); - strncpy(ofulldir, output_directory, MAXPGPATH); + strlcpy(ifulldir, input_directory, MAXPGPATH); + strlcpy(ofulldir, output_directory, MAXPGPATH); if (OidIsValid(tsoid)) snprintf(manifest_prefix, MAXPGPATH, "pg_tblspc/%u/", tsoid); else @@ -855,7 +855,7 @@ process_directory_recursively(Oid tsoid, /* Append new pathname component to relative path. */ if (relative_path == NULL) - strncpy(new_relative_path, de->d_name, MAXPGPATH); + strlcpy(new_relative_path, de->d_name, MAXPGPATH); else snprintf(new_relative_path, MAXPGPATH, "%s/%s", relative_path, de->d_name); @@ -1131,7 +1131,7 @@ scan_for_existing_tablespaces(char *pathname, cb_options *opt) pg_log_debug("scanning \"%s\"", pg_tblspc); if ((dir = opendir(pg_tblspc)) == NULL) - pg_fatal("could not open directory \"%s\": %m", pathname); + pg_fatal("could not open directory \"%s\": %m", pg_tblspc); while (errno = 0, (de = readdir(dir)) != NULL) { @@ -1203,8 +1203,8 @@ scan_for_existing_tablespaces(char *pathname, cb_options *opt) { if (strcmp(tsmap->old_dir, link_target) == 0) { - strncpy(ts->old_dir, tsmap->old_dir, MAXPGPATH); - strncpy(ts->new_dir, tsmap->new_dir, MAXPGPATH); + strlcpy(ts->old_dir, tsmap->old_dir, MAXPGPATH); + strlcpy(ts->new_dir, tsmap->new_dir, MAXPGPATH); ts->in_place = false; break; } @@ -1238,6 +1238,9 @@ scan_for_existing_tablespaces(char *pathname, cb_options *opt) tslist = ts; } + if (closedir(dir) != 0) + pg_fatal("could not close directory \"%s\": %m", pg_tblspc); + return tslist; } diff --git a/src/bin/pg_combinebackup/reconstruct.c b/src/bin/pg_combinebackup/reconstruct.c index b835ec363e..873d307902 100644 --- a/src/bin/pg_combinebackup/reconstruct.c +++ b/src/bin/pg_combinebackup/reconstruct.c @@ -577,13 +577,12 @@ write_reconstructed_file(char *input_filename, { if (current_block == start_of_range) appendStringInfo(&debug_buf, " %u:%s@" UINT64_FORMAT, - current_block, - s == NULL ? "ZERO" : s->filename, + current_block, s->filename, (uint64) offsetmap[current_block]); else appendStringInfo(&debug_buf, " %u-%u:%s@" UINT64_FORMAT, start_of_range, current_block, - s == NULL ? "ZERO" : s->filename, + s->filename, (uint64) offsetmap[current_block]); } diff --git a/src/bin/pg_combinebackup/write_manifest.c b/src/bin/pg_combinebackup/write_manifest.c index 608e84f3b4..01deb82cc9 100644 --- a/src/bin/pg_combinebackup/write_manifest.c +++ b/src/bin/pg_combinebackup/write_manifest.c @@ -241,8 +241,6 @@ escape_json(StringInfo buf, const char *str) static void flush_manifest(manifest_writer *mwriter) { - char pathname[MAXPGPATH]; - if (mwriter->fd == -1 && (mwriter->fd = open(mwriter->pathname, O_WRONLY | O_CREAT | O_EXCL | PG_BINARY, @@ -260,13 +258,15 @@ flush_manifest(manifest_writer *mwriter) pg_fatal("could not write \"%s\": %m", mwriter->pathname); else pg_fatal("could not write file \"%s\": wrote only %d of %d bytes", - pathname, (int) wb, mwriter->buf.len); + mwriter->pathname, (int) wb, mwriter->buf.len); } - if (mwriter->still_checksumming) + if (mwriter->still_checksumming && pg_checksum_update(&mwriter->manifest_ctx, (uint8 *) mwriter->buf.data, - mwriter->buf.len); + mwriter->buf.len) < 0) + pg_fatal("could not update checksum of file \"%s\"", + mwriter->pathname); resetStringInfo(&mwriter->buf); } } -- 2.39.3 (Apple Git-145)