From a4ff24df41626ef543fbbb05cd47bc7eed0278a6 Mon Sep 17 00:00:00 2001 From: Andreas Karlsson Date: Fri, 6 Jun 2025 16:05:00 +0200 Subject: [PATCH] Use OpenTransientFile() instead of BasicOpenFile() By using OpenTransientFile() we do not have to close file descriptors on error plus PostgreSQL will check if we have forgot to close any files on commit. This change made us find one instance where we had forgot to close a file which is also fixed in this commit. --- contrib/pg_tde/src/access/pg_tde_tdemap.c | 30 +++++++++++------------ contrib/pg_tde/src/catalog/tde_keyring.c | 20 ++++++--------- contrib/pg_tde/src/include/pg_tde_fe.h | 3 ++- contrib/pg_tde/src/keyring/keyring_file.c | 20 +++++++-------- 4 files changed, 34 insertions(+), 39 deletions(-) diff --git a/contrib/pg_tde/src/access/pg_tde_tdemap.c b/contrib/pg_tde/src/access/pg_tde_tdemap.c index d54784ce02959..0137b9e95ddc7 100644 --- a/contrib/pg_tde/src/access/pg_tde_tdemap.c +++ b/contrib/pg_tde/src/access/pg_tde_tdemap.c @@ -177,7 +177,7 @@ pg_tde_save_principal_key_redo(const TDESignedPrincipalKeyInfo *signed_key_info) LWLockAcquire(tde_lwlock_enc_keys(), LW_EXCLUSIVE); map_fd = pg_tde_open_file_write(db_map_path, signed_key_info, false, &curr_pos); - close(map_fd); + CloseTransientFile(map_fd); LWLockRelease(tde_lwlock_enc_keys()); } @@ -216,7 +216,7 @@ pg_tde_save_principal_key(const TDEPrincipalKey *principal_key, bool write_xlog) } map_fd = pg_tde_open_file_write(db_map_path, &signed_key_Info, true, &curr_pos); - close(map_fd); + CloseTransientFile(map_fd); } /* @@ -365,7 +365,7 @@ pg_tde_write_key_map_entry(const RelFileLocator *rlocator, const InternalKey *re /* Write the given entry at curr_pos; i.e. the free entry. */ pg_tde_write_one_map_entry(map_fd, &write_map_entry, &curr_pos, db_map_path); - close(map_fd); + CloseTransientFile(map_fd); } /* @@ -410,7 +410,7 @@ pg_tde_free_key_map_entry(const RelFileLocator rlocator) } } - close(map_fd); + CloseTransientFile(map_fd); LWLockRelease(tde_lwlock_enc_keys()); } @@ -490,8 +490,8 @@ pg_tde_perform_rotate_key(TDEPrincipalKey *principal_key, TDEPrincipalKey *new_p pfree(rel_key_data); } - close(old_fd); - close(new_fd); + CloseTransientFile(old_fd); + CloseTransientFile(new_fd); /* * Do the final steps - replace the current _map with the file with new @@ -589,7 +589,7 @@ pg_tde_wal_last_key_set_lsn(XLogRecPtr lsn, const char *keyfile_path) } LWLockRelease(lock_pk); - close(fd); + CloseTransientFile(fd); } /* @@ -649,7 +649,7 @@ pg_tde_find_map_entry(const RelFileLocator *rlocator, TDEMapEntryType key_type, } } - close(map_fd); + CloseTransientFile(map_fd); return found; } @@ -688,7 +688,7 @@ pg_tde_count_relations(Oid dbOid) count++; } - close(map_fd); + CloseTransientFile(map_fd); LWLockRelease(lock_pk); @@ -764,7 +764,7 @@ pg_tde_open_file_basic(const char *tde_filename, int fileFlags, bool ignore_miss { int fd; - fd = BasicOpenFile(tde_filename, fileFlags); + fd = OpenTransientFile(tde_filename, fileFlags); if (fd < 0 && !(errno == ENOENT && ignore_missing == true)) { ereport(ERROR, @@ -792,7 +792,6 @@ pg_tde_file_header_read(const char *tde_filename, int fd, TDEFileHeader *fheader if (*bytes_read != TDE_FILE_HEADER_SIZE || fheader->file_version != PG_TDE_FILEMAGIC) { - close(fd); ereport(FATAL, errcode_for_file_access(), errmsg("TDE map file \"%s\" is corrupted: %m", tde_filename)); @@ -870,7 +869,7 @@ pg_tde_get_principal_key_info(Oid dbOid) pg_tde_file_header_read(db_map_path, fd, &fheader, &bytes_read); - close(fd); + CloseTransientFile(fd); /* * It's not a new file. So we can copy the principal key info from the @@ -1008,6 +1007,7 @@ pg_tde_read_last_wal_key(void) if (fsize == TDE_FILE_HEADER_SIZE) { LWLockRelease(lock_pk); + CloseTransientFile(fd); return NULL; } @@ -1016,7 +1016,7 @@ pg_tde_read_last_wal_key(void) rel_key_data = tde_decrypt_rel_key(principal_key, &map_entry); LWLockRelease(lock_pk); - close(fd); + CloseTransientFile(fd); return rel_key_data; } @@ -1064,7 +1064,7 @@ pg_tde_fetch_wal_keys(XLogRecPtr start_lsn) wal_rec = pg_tde_add_wal_key_to_cache(&stub_key, InvalidXLogRecPtr); LWLockRelease(lock_pk); - close(fd); + CloseTransientFile(fd); return wal_rec; } @@ -1094,7 +1094,7 @@ pg_tde_fetch_wal_keys(XLogRecPtr start_lsn) } } LWLockRelease(lock_pk); - close(fd); + CloseTransientFile(fd); return return_wal_rec; } diff --git a/contrib/pg_tde/src/catalog/tde_keyring.c b/contrib/pg_tde/src/catalog/tde_keyring.c index 6ddde83343fb1..742bb5d2f8ebe 100644 --- a/contrib/pg_tde/src/catalog/tde_keyring.c +++ b/contrib/pg_tde/src/catalog/tde_keyring.c @@ -476,13 +476,12 @@ save_new_key_provider_info(KeyringProviderRecord *provider, Oid databaseId) if (strcmp(existing_provider.provider_name, provider->provider_name) == 0) { - close(fd); ereport(ERROR, errcode(ERRCODE_DUPLICATE_OBJECT), errmsg("Key provider \"%s\" already exists.", provider->provider_name)); } } - close(fd); + CloseTransientFile(fd); if (max_provider_id == PG_INT32_MAX) { @@ -610,7 +609,7 @@ write_key_provider_info(KeyringProviderRecordInFile *record, bool write_xlog) Assert(LWLockHeldByMeInMode(tde_provider_info_lock(), LW_EXCLUSIVE)); get_keyring_infofile_path(kp_info_path, record->database_id); - fd = BasicOpenFile(kp_info_path, O_CREAT | O_RDWR | PG_BINARY); + fd = OpenTransientFile(kp_info_path, O_CREAT | O_RDWR | PG_BINARY); if (fd < 0) { ereport(ERROR, @@ -638,7 +637,6 @@ write_key_provider_info(KeyringProviderRecordInFile *record, bool write_xlog) record->offset_in_file); if (bytes_written != sizeof(KeyringProviderRecord)) { - close(fd); ereport(ERROR, errcode_for_file_access(), errmsg("key provider info file \"%s\" can't be written: %m", @@ -646,12 +644,11 @@ write_key_provider_info(KeyringProviderRecordInFile *record, bool write_xlog) } if (pg_fsync(fd) != 0) { - close(fd); ereport(ERROR, errcode_for_file_access(), errmsg("could not fsync file \"%s\": %m", kp_info_path)); } - close(fd); + CloseTransientFile(fd); } /* Returns true if the record is found, false otherwise. */ @@ -678,7 +675,7 @@ get_keyring_info_file_record_by_name(char *provider_name, Oid database_id, record->database_id = database_id; record->offset_in_file = current_file_offset; record->provider = existing_provider; - close(fd); + CloseTransientFile(fd); return true; } @@ -686,7 +683,7 @@ get_keyring_info_file_record_by_name(char *provider_name, Oid database_id, } /* No matching key provider found */ - close(fd); + CloseTransientFile(fd); return false; } @@ -750,7 +747,7 @@ scan_key_provider_file(ProviderScanType scanType, void *scanKey, Oid dbOid) LWLockAcquire(tde_provider_info_lock(), LW_SHARED); - fd = BasicOpenFile(kp_info_path, PG_BINARY); + fd = OpenTransientFile(kp_info_path, PG_BINARY); if (fd < 0) { LWLockRelease(tde_provider_info_lock()); @@ -801,7 +798,7 @@ scan_key_provider_file(ProviderScanType scanType, void *scanKey, Oid dbOid) } } } - close(fd); + CloseTransientFile(fd); LWLockRelease(tde_provider_info_lock()); return providers_list; } @@ -994,7 +991,7 @@ open_keyring_infofile(Oid database_id, int flags) char kp_info_path[MAXPGPATH]; get_keyring_infofile_path(kp_info_path, database_id); - fd = BasicOpenFile(kp_info_path, flags | PG_BINARY); + fd = OpenTransientFile(kp_info_path, flags | PG_BINARY); if (fd < 0) { ereport(ERROR, @@ -1022,7 +1019,6 @@ fetch_next_key_provider(int fd, off_t *curr_pos, KeyringProviderRecord *provider return false; if (bytes_read != sizeof(KeyringProviderRecord)) { - close(fd); /* Corrupt file */ ereport(ERROR, errcode_for_file_access(), diff --git a/contrib/pg_tde/src/include/pg_tde_fe.h b/contrib/pg_tde/src/include/pg_tde_fe.h index 53269ea9123be..196457a052288 100644 --- a/contrib/pg_tde/src/include/pg_tde_fe.h +++ b/contrib/pg_tde/src/include/pg_tde_fe.h @@ -85,7 +85,8 @@ static int tde_fe_error_level = 0; #define LW_EXCLUSIVE NULL #define tde_lwlock_enc_keys() NULL -#define BasicOpenFile(fileName, fileFlags) open(fileName, fileFlags, PG_FILE_MODE_OWNER) +#define OpenTransientFile(fileName, fileFlags) open(fileName, fileFlags, PG_FILE_MODE_OWNER) +#define CloseTransientFile(fd) close(fd) #define AllocateFile(name, mode) fopen(name, mode) #define FreeFile(file) fclose(file) diff --git a/contrib/pg_tde/src/keyring/keyring_file.c b/contrib/pg_tde/src/keyring/keyring_file.c index 5d1f83c1349ef..2166b8ef95442 100644 --- a/contrib/pg_tde/src/keyring/keyring_file.c +++ b/contrib/pg_tde/src/keyring/keyring_file.c @@ -53,7 +53,7 @@ get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCode *return_code = KEYRING_CODE_SUCCESS; - fd = BasicOpenFile(file_keyring->file_name, PG_BINARY); + fd = OpenTransientFile(file_keyring->file_name, PG_BINARY); if (fd < 0) return NULL; @@ -69,13 +69,13 @@ get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCode * Empty keyring file is considered as a valid keyring file that * has no keys */ - close(fd); + CloseTransientFile(fd); pfree(key); return NULL; } if (bytes_read != sizeof(KeyInfo)) { - close(fd); + CloseTransientFile(fd); pfree(key); /* Corrupt file */ *return_code = KEYRING_CODE_DATA_CORRUPTED; @@ -88,11 +88,11 @@ get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCode } if (strncasecmp(key->name, key_name, sizeof(key->name)) == 0) { - close(fd); + CloseTransientFile(fd); return key; } } - close(fd); + CloseTransientFile(fd); pfree(key); return NULL; } @@ -116,7 +116,7 @@ set_key_by_name(GenericKeyring *keyring, KeyInfo *key) errmsg("Key with name %s already exists in keyring", key->name)); } - fd = BasicOpenFile(file_keyring->file_name, O_CREAT | O_RDWR | PG_BINARY); + fd = OpenTransientFile(file_keyring->file_name, O_CREAT | O_RDWR | PG_BINARY); if (fd < 0) { ereport(ERROR, @@ -128,7 +128,6 @@ set_key_by_name(GenericKeyring *keyring, KeyInfo *key) bytes_written = pg_pwrite(fd, key, sizeof(KeyInfo), curr_pos); if (bytes_written != sizeof(KeyInfo)) { - close(fd); ereport(ERROR, errcode_for_file_access(), errmsg("keyring file \"%s\" can't be written: %m", @@ -137,20 +136,19 @@ set_key_by_name(GenericKeyring *keyring, KeyInfo *key) if (pg_fsync(fd) != 0) { - close(fd); ereport(ERROR, errcode_for_file_access(), errmsg("could not fsync file \"%s\": %m", file_keyring->file_name)); } - close(fd); + CloseTransientFile(fd); } static void validate(GenericKeyring *keyring) { FileKeyring *file_keyring = (FileKeyring *) keyring; - int fd = BasicOpenFile(file_keyring->file_name, O_CREAT | O_RDWR | PG_BINARY); + int fd = OpenTransientFile(file_keyring->file_name, O_CREAT | O_RDWR | PG_BINARY); if (fd < 0) { @@ -159,5 +157,5 @@ validate(GenericKeyring *keyring) errmsg("Failed to open keyring file %s: %m", file_keyring->file_name)); } - close(fd); + CloseTransientFile(fd); }