Skip to content

Commit e636b10

Browse files
jeltzjeltz
committed
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.
1 parent 2feea53 commit e636b10

File tree

4 files changed

+34
-39
lines changed

4 files changed

+34
-39
lines changed

‎contrib/pg_tde/src/access/pg_tde_tdemap.c

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ pg_tde_save_principal_key_redo(const TDESignedPrincipalKeyInfo *signed_key_info)
177177
LWLockAcquire(tde_lwlock_enc_keys(), LW_EXCLUSIVE);
178178

179179
map_fd = pg_tde_open_file_write(db_map_path, signed_key_info, false, &curr_pos);
180-
close(map_fd);
180+
CloseTransientFile(map_fd);
181181

182182
LWLockRelease(tde_lwlock_enc_keys());
183183
}
@@ -216,7 +216,7 @@ pg_tde_save_principal_key(const TDEPrincipalKey *principal_key, bool write_xlog)
216216
}
217217

218218
map_fd = pg_tde_open_file_write(db_map_path, &signed_key_Info, true, &curr_pos);
219-
close(map_fd);
219+
CloseTransientFile(map_fd);
220220
}
221221

222222
/*
@@ -365,7 +365,7 @@ pg_tde_write_key_map_entry(const RelFileLocator *rlocator, const InternalKey *re
365365
/* Write the given entry at curr_pos; i.e. the free entry. */
366366
pg_tde_write_one_map_entry(map_fd, &write_map_entry, &curr_pos, db_map_path);
367367

368-
close(map_fd);
368+
CloseTransientFile(map_fd);
369369
}
370370

371371
/*
@@ -410,7 +410,7 @@ pg_tde_free_key_map_entry(const RelFileLocator rlocator)
410410
}
411411
}
412412

413-
close(map_fd);
413+
CloseTransientFile(map_fd);
414414

415415
LWLockRelease(tde_lwlock_enc_keys());
416416
}
@@ -490,8 +490,8 @@ pg_tde_perform_rotate_key(TDEPrincipalKey *principal_key, TDEPrincipalKey *new_p
490490
pfree(rel_key_data);
491491
}
492492

493-
close(old_fd);
494-
close(new_fd);
493+
CloseTransientFile(old_fd);
494+
CloseTransientFile(new_fd);
495495

496496
/*
497497
* 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)
589589
}
590590

591591
LWLockRelease(lock_pk);
592-
close(fd);
592+
CloseTransientFile(fd);
593593
}
594594

595595
/*
@@ -649,7 +649,7 @@ pg_tde_find_map_entry(const RelFileLocator *rlocator, TDEMapEntryType key_type,
649649
}
650650
}
651651

652-
close(map_fd);
652+
CloseTransientFile(map_fd);
653653

654654
return found;
655655
}
@@ -688,7 +688,7 @@ pg_tde_count_relations(Oid dbOid)
688688
count++;
689689
}
690690

691-
close(map_fd);
691+
CloseTransientFile(map_fd);
692692

693693
LWLockRelease(lock_pk);
694694

@@ -764,7 +764,7 @@ pg_tde_open_file_basic(const char *tde_filename, int fileFlags, bool ignore_miss
764764
{
765765
int fd;
766766

767-
fd = BasicOpenFile(tde_filename, fileFlags);
767+
fd = OpenTransientFile(tde_filename, fileFlags);
768768
if (fd < 0 && !(errno == ENOENT && ignore_missing == true))
769769
{
770770
ereport(ERROR,
@@ -792,7 +792,6 @@ pg_tde_file_header_read(const char *tde_filename, int fd, TDEFileHeader *fheader
792792
if (*bytes_read != TDE_FILE_HEADER_SIZE
793793
|| fheader->file_version != PG_TDE_FILEMAGIC)
794794
{
795-
close(fd);
796795
ereport(FATAL,
797796
errcode_for_file_access(),
798797
errmsg("TDE map file \"%s\" is corrupted: %m", tde_filename));
@@ -870,7 +869,7 @@ pg_tde_get_principal_key_info(Oid dbOid)
870869

871870
pg_tde_file_header_read(db_map_path, fd, &fheader, &bytes_read);
872871

873-
close(fd);
872+
CloseTransientFile(fd);
874873

875874
/*
876875
* 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)
10081007
if (fsize == TDE_FILE_HEADER_SIZE)
10091008
{
10101009
LWLockRelease(lock_pk);
1010+
CloseTransientFile(fd);
10111011
return NULL;
10121012
}
10131013

@@ -1016,7 +1016,7 @@ pg_tde_read_last_wal_key(void)
10161016

10171017
rel_key_data = tde_decrypt_rel_key(principal_key, &map_entry);
10181018
LWLockRelease(lock_pk);
1019-
close(fd);
1019+
CloseTransientFile(fd);
10201020

10211021
return rel_key_data;
10221022
}
@@ -1064,7 +1064,7 @@ pg_tde_fetch_wal_keys(XLogRecPtr start_lsn)
10641064
wal_rec = pg_tde_add_wal_key_to_cache(&stub_key, InvalidXLogRecPtr);
10651065

10661066
LWLockRelease(lock_pk);
1067-
close(fd);
1067+
CloseTransientFile(fd);
10681068
return wal_rec;
10691069
}
10701070

@@ -1094,7 +1094,7 @@ pg_tde_fetch_wal_keys(XLogRecPtr start_lsn)
10941094
}
10951095
}
10961096
LWLockRelease(lock_pk);
1097-
close(fd);
1097+
CloseTransientFile(fd);
10981098

10991099
return return_wal_rec;
11001100
}

‎contrib/pg_tde/src/catalog/tde_keyring.c

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -476,13 +476,12 @@ save_new_key_provider_info(KeyringProviderRecord *provider, Oid databaseId)
476476

477477
if (strcmp(existing_provider.provider_name, provider->provider_name) == 0)
478478
{
479-
close(fd);
480479
ereport(ERROR,
481480
errcode(ERRCODE_DUPLICATE_OBJECT),
482481
errmsg("Key provider \"%s\" already exists.", provider->provider_name));
483482
}
484483
}
485-
close(fd);
484+
CloseTransientFile(fd);
486485

487486
if (max_provider_id == PG_INT32_MAX)
488487
{
@@ -610,7 +609,7 @@ write_key_provider_info(KeyringProviderRecordInFile *record, bool write_xlog)
610609
Assert(LWLockHeldByMeInMode(tde_provider_info_lock(), LW_EXCLUSIVE));
611610

612611
get_keyring_infofile_path(kp_info_path, record->database_id);
613-
fd = BasicOpenFile(kp_info_path, O_CREAT | O_RDWR | PG_BINARY);
612+
fd = OpenTransientFile(kp_info_path, O_CREAT | O_RDWR | PG_BINARY);
614613
if (fd < 0)
615614
{
616615
ereport(ERROR,
@@ -638,20 +637,18 @@ write_key_provider_info(KeyringProviderRecordInFile *record, bool write_xlog)
638637
record->offset_in_file);
639638
if (bytes_written != sizeof(KeyringProviderRecord))
640639
{
641-
close(fd);
642640
ereport(ERROR,
643641
errcode_for_file_access(),
644642
errmsg("key provider info file \"%s\" can't be written: %m",
645643
kp_info_path));
646644
}
647645
if (pg_fsync(fd) != 0)
648646
{
649-
close(fd);
650647
ereport(ERROR,
651648
errcode_for_file_access(),
652649
errmsg("could not fsync file \"%s\": %m", kp_info_path));
653650
}
654-
close(fd);
651+
CloseTransientFile(fd);
655652
}
656653

657654
/* Returns true if the record is found, false otherwise. */
@@ -678,15 +675,15 @@ get_keyring_info_file_record_by_name(char *provider_name, Oid database_id,
678675
record->database_id = database_id;
679676
record->offset_in_file = current_file_offset;
680677
record->provider = existing_provider;
681-
close(fd);
678+
CloseTransientFile(fd);
682679
return true;
683680
}
684681

685682
current_file_offset = next_file_offset;
686683
}
687684

688685
/* No matching key provider found */
689-
close(fd);
686+
CloseTransientFile(fd);
690687
return false;
691688
}
692689

@@ -750,7 +747,7 @@ scan_key_provider_file(ProviderScanType scanType, void *scanKey, Oid dbOid)
750747

751748
LWLockAcquire(tde_provider_info_lock(), LW_SHARED);
752749

753-
fd = BasicOpenFile(kp_info_path, PG_BINARY);
750+
fd = OpenTransientFile(kp_info_path, PG_BINARY);
754751
if (fd < 0)
755752
{
756753
LWLockRelease(tde_provider_info_lock());
@@ -801,7 +798,7 @@ scan_key_provider_file(ProviderScanType scanType, void *scanKey, Oid dbOid)
801798
}
802799
}
803800
}
804-
close(fd);
801+
CloseTransientFile(fd);
805802
LWLockRelease(tde_provider_info_lock());
806803
return providers_list;
807804
}
@@ -994,7 +991,7 @@ open_keyring_infofile(Oid database_id, int flags)
994991
char kp_info_path[MAXPGPATH];
995992

996993
get_keyring_infofile_path(kp_info_path, database_id);
997-
fd = BasicOpenFile(kp_info_path, flags | PG_BINARY);
994+
fd = OpenTransientFile(kp_info_path, flags | PG_BINARY);
998995
if (fd < 0)
999996
{
1000997
ereport(ERROR,
@@ -1022,7 +1019,6 @@ fetch_next_key_provider(int fd, off_t *curr_pos, KeyringProviderRecord *provider
10221019
return false;
10231020
if (bytes_read != sizeof(KeyringProviderRecord))
10241021
{
1025-
close(fd);
10261022
/* Corrupt file */
10271023
ereport(ERROR,
10281024
errcode_for_file_access(),

‎contrib/pg_tde/src/include/pg_tde_fe.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,8 @@ static int tde_fe_error_level = 0;
8585
#define LW_EXCLUSIVE NULL
8686
#define tde_lwlock_enc_keys() NULL
8787

88-
#define BasicOpenFile(fileName, fileFlags) open(fileName, fileFlags, PG_FILE_MODE_OWNER)
88+
#define OpenTransientFile(fileName, fileFlags) open(fileName, fileFlags, PG_FILE_MODE_OWNER)
89+
#define CloseTransientFile(fd) close(fd)
8990
#define AllocateFile(name, mode) fopen(name, mode)
9091
#define FreeFile(file) fclose(file)
9192

‎contrib/pg_tde/src/keyring/keyring_file.c

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCode
5353

5454
*return_code = KEYRING_CODE_SUCCESS;
5555

56-
fd = BasicOpenFile(file_keyring->file_name, PG_BINARY);
56+
fd = OpenTransientFile(file_keyring->file_name, PG_BINARY);
5757
if (fd < 0)
5858
return NULL;
5959

@@ -69,13 +69,13 @@ get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCode
6969
* Empty keyring file is considered as a valid keyring file that
7070
* has no keys
7171
*/
72-
close(fd);
72+
CloseTransientFile(fd);
7373
pfree(key);
7474
return NULL;
7575
}
7676
if (bytes_read != sizeof(KeyInfo))
7777
{
78-
close(fd);
78+
CloseTransientFile(fd);
7979
pfree(key);
8080
/* Corrupt file */
8181
*return_code = KEYRING_CODE_DATA_CORRUPTED;
@@ -88,11 +88,11 @@ get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCode
8888
}
8989
if (strncasecmp(key->name, key_name, sizeof(key->name)) == 0)
9090
{
91-
close(fd);
91+
CloseTransientFile(fd);
9292
return key;
9393
}
9494
}
95-
close(fd);
95+
CloseTransientFile(fd);
9696
pfree(key);
9797
return NULL;
9898
}
@@ -116,7 +116,7 @@ set_key_by_name(GenericKeyring *keyring, KeyInfo *key)
116116
errmsg("Key with name %s already exists in keyring", key->name));
117117
}
118118

119-
fd = BasicOpenFile(file_keyring->file_name, O_CREAT | O_RDWR | PG_BINARY);
119+
fd = OpenTransientFile(file_keyring->file_name, O_CREAT | O_RDWR | PG_BINARY);
120120
if (fd < 0)
121121
{
122122
ereport(ERROR,
@@ -128,7 +128,6 @@ set_key_by_name(GenericKeyring *keyring, KeyInfo *key)
128128
bytes_written = pg_pwrite(fd, key, sizeof(KeyInfo), curr_pos);
129129
if (bytes_written != sizeof(KeyInfo))
130130
{
131-
close(fd);
132131
ereport(ERROR,
133132
errcode_for_file_access(),
134133
errmsg("keyring file \"%s\" can't be written: %m",
@@ -137,20 +136,19 @@ set_key_by_name(GenericKeyring *keyring, KeyInfo *key)
137136

138137
if (pg_fsync(fd) != 0)
139138
{
140-
close(fd);
141139
ereport(ERROR,
142140
errcode_for_file_access(),
143141
errmsg("could not fsync file \"%s\": %m",
144142
file_keyring->file_name));
145143
}
146-
close(fd);
144+
CloseTransientFile(fd);
147145
}
148146

149147
static void
150148
validate(GenericKeyring *keyring)
151149
{
152150
FileKeyring *file_keyring = (FileKeyring *) keyring;
153-
int fd = BasicOpenFile(file_keyring->file_name, O_CREAT | O_RDWR | PG_BINARY);
151+
int fd = OpenTransientFile(file_keyring->file_name, O_CREAT | O_RDWR | PG_BINARY);
154152

155153
if (fd < 0)
156154
{
@@ -159,5 +157,5 @@ validate(GenericKeyring *keyring)
159157
errmsg("Failed to open keyring file %s: %m", file_keyring->file_name));
160158
}
161159

162-
close(fd);
160+
CloseTransientFile(fd);
163161
}

0 commit comments

Comments
 (0)