Skip to content

Commit 3c5511d

Browse files
committed
Tidy sanitizers' suppressions list and fix some mem leaks
Tihs PR makes a suppression list as specific as possible, so it won't cover up new issues. And adds comments to existing suppressions. Also, fixes memory leaks in bin/pgctl code (all related to frontend usage) and low-hanging fruits in pgctl.
1 parent 34e2d29 commit 3c5511d

File tree

8 files changed

+119
-12
lines changed

8 files changed

+119
-12
lines changed

ci_scripts/suppressions/lsan.supp

Lines changed: 45 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,50 @@
1+
# external submodule
2+
leak:kmip.c
3+
4+
# the parser of command line arguments of the backend
15
leak:save_ps_display_args
6+
7+
# a bunch of not freed addrinfo sctructs, escaped values etc.
28
leak:initdb.c
3-
leak:fe_memutils.c
4-
leak:fe-exec.c
5-
leak:fe-connect.c
6-
leak:pqexpbuffer.c
7-
leak:strdup
8-
leak:preproc.y
9-
leak:pgtypeslib/common.c
10-
leak:ecpglib/memory.c
11-
leak:kmip.c
9+
10+
# shlibs loaded with dlsym() never gets dlclose'd
11+
# TODO: needs an investigation as it is down the call stack
12+
# of exec_simple_query->PortalRun
13+
leak:internal_load_library
14+
15+
# pg_config utility does not free configdata
16+
leak:pg_config/pg_config.c
17+
18+
# A psprintf() result assigned to the global var pgdata_opt
19+
leak:bin/pg_ctl/pg_ctl.c
20+
21+
# TODO: A couple of not freed char *. Whilst wit's trivially to fix
22+
# meson complains with "maybe-uninitialized" on free()
23+
leak:bin/psql/describe.c
24+
25+
# options parsing during the regressions start
26+
leak:regression_main
27+
28+
# Static funcs in /bin/psql/startup.c are used
29+
# during the start to parse options
30+
leak:simple_action_list_append
31+
leak:parse_psql_options
32+
33+
# A bunch of parameters, options and identifiers are not being freed.
34+
#
35+
# TODO?: ReceiveArchiveStreamChunk->CreateBackupStreamer down in the
36+
# callstak creates a streamer that is never freed. It's being called in a loop
37+
# but it looks like it is not a repetitive action and rather happens once per
38+
# receiving CopyData message from server.
39+
#
40+
# bin/pg_basebackup/pg_basebackup.c
41+
leak:BaseBackup
42+
43+
# json parser does not free `fname` on the frontend
44+
leak:parse_object_field
45+
46+
# does not free a bunch of parsed flags
47+
leak:bin/pg_waldump/pg_waldump.c
1248

1349
#pg_dump
1450
leak:getSchemaData

contrib/pg_tde/src/access/pg_tde_tdemap.c

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1077,6 +1077,10 @@ pg_tde_fetch_wal_keys(XLogRecPtr start_lsn)
10771077

10781078
wal_rec = pg_tde_add_wal_key_to_cache(&stub_key, InvalidXLogRecPtr);
10791079

1080+
#ifdef FRONTEND
1081+
/* The backen frees it after copying to the cache. */
1082+
pfree(principal_key);
1083+
#endif
10801084
LWLockRelease(lock_pk);
10811085
CloseTransientFile(fd);
10821086
return wal_rec;
@@ -1107,6 +1111,9 @@ pg_tde_fetch_wal_keys(XLogRecPtr start_lsn)
11071111
return_wal_rec = wal_rec;
11081112
}
11091113
}
1114+
#ifdef FRONTEND
1115+
pfree(principal_key);
1116+
#endif
11101117
LWLockRelease(lock_pk);
11111118
CloseTransientFile(fd);
11121119

contrib/pg_tde/src/catalog/tde_keyring.c

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include "catalog/tde_keyring.h"
2121
#include "catalog/tde_principal_key.h"
2222
#include "common/pg_tde_utils.h"
23+
#include "keyring/keyring_api.h"
2324
#include "pg_tde.h"
2425

2526
#ifndef FRONTEND
@@ -562,6 +563,51 @@ check_provider_record(KeyringProviderRecord *provider_record)
562563

563564
#endif /* !FRONTEND */
564565

566+
void
567+
free_keyring(GenericKeyring *keyring)
568+
{
569+
FileKeyring *file = (FileKeyring *) keyring;
570+
VaultV2Keyring *vault = (VaultV2Keyring *) keyring;
571+
KmipKeyring *kmip = (KmipKeyring *) keyring;
572+
573+
switch (keyring->type)
574+
{
575+
case FILE_KEY_PROVIDER:
576+
if (file->file_name)
577+
pfree(file->file_name);
578+
break;
579+
case VAULT_V2_KEY_PROVIDER:
580+
if (vault->vault_ca_path)
581+
pfree(vault->vault_ca_path);
582+
if (vault->vault_mount_path)
583+
pfree(vault->vault_mount_path);
584+
if (vault->vault_token)
585+
pfree(vault->vault_token);
586+
if (vault->vault_token_path)
587+
pfree(vault->vault_token_path);
588+
if (vault->vault_url)
589+
pfree(vault->vault_url);
590+
break;
591+
case KMIP_KEY_PROVIDER:
592+
if (kmip->kmip_ca_path)
593+
pfree(kmip->kmip_ca_path);
594+
if (kmip->kmip_cert_path)
595+
pfree(kmip->kmip_cert_path);
596+
if (kmip->kmip_host)
597+
pfree(kmip->kmip_host);
598+
if (kmip->kmip_key_path)
599+
pfree(kmip->kmip_key_path);
600+
if (kmip->kmip_port)
601+
pfree(kmip->kmip_port);
602+
break;
603+
default:
604+
Assert(false);
605+
break;
606+
}
607+
608+
pfree(keyring);
609+
}
610+
565611
void
566612
write_key_provider_info(KeyringProviderRecordInFile *record, bool write_xlog)
567613
{
@@ -682,6 +728,8 @@ simple_list_free(SimplePtrList *list)
682728
pfree(cell);
683729
cell = next;
684730
}
731+
732+
pfree(list);
685733
}
686734
#endif /* FRONTEND */
687735

@@ -818,6 +866,8 @@ load_file_keyring_provider_options(char *keyring_options)
818866
ereport(WARNING,
819867
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
820868
errmsg("file path is missing in the keyring options"));
869+
870+
free_keyring((GenericKeyring *) file_keyring);
821871
return NULL;
822872
}
823873

@@ -845,6 +895,8 @@ load_vaultV2_keyring_provider_options(char *keyring_options)
845895
(vaultV2_keyring->vault_token_path != NULL && vaultV2_keyring->vault_token_path[0] != '\0') ? "" : " tokenPath",
846896
(vaultV2_keyring->vault_url != NULL && vaultV2_keyring->vault_url[0] != '\0') ? "" : " url",
847897
(vaultV2_keyring->vault_mount_path != NULL && vaultV2_keyring->vault_mount_path[0] != '\0') ? "" : " mountPath"));
898+
899+
free_keyring((GenericKeyring *) vaultV2_keyring);
848900
return NULL;
849901
}
850902

@@ -878,6 +930,8 @@ load_kmip_keyring_provider_options(char *keyring_options)
878930
(kmip_keyring->kmip_ca_path != NULL && kmip_keyring->kmip_ca_path[0] != '\0') ? "" : " caPath",
879931
(kmip_keyring->kmip_cert_path != NULL && kmip_keyring->kmip_cert_path[0] != '\0') ? "" : " certPath",
880932
(kmip_keyring->kmip_key_path != NULL && kmip_keyring->kmip_key_path[0] != '\0') ? "" : " keyPath"));
933+
934+
free_keyring((GenericKeyring *) kmip_keyring);
881935
return NULL;
882936
}
883937

@@ -946,7 +1000,10 @@ debug_print_kerying(GenericKeyring *keyring)
9461000
static inline void
9471001
get_keyring_infofile_path(char *resPath, Oid dbOid)
9481002
{
949-
join_path_components(resPath, pg_tde_get_data_dir(), psprintf(PG_TDE_KEYRING_FILENAME, dbOid));
1003+
char *fname = psprintf(PG_TDE_KEYRING_FILENAME, dbOid);
1004+
1005+
join_path_components(resPath, pg_tde_get_data_dir(), fname);
1006+
pfree(fname);
9501007
}
9511008

9521009
static int

contrib/pg_tde/src/catalog/tde_principal_key.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -863,7 +863,7 @@ get_principal_key_from_keyring(Oid dbOid)
863863
Assert(dbOid == principalKey->keyInfo.databaseId);
864864

865865
pfree(keyInfo);
866-
pfree(keyring);
866+
free_keyring(keyring);
867867
pfree(principalKeyInfo);
868868

869869
return principalKey;

contrib/pg_tde/src/include/access/pg_tde_tdemap.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,10 @@ extern void pg_tde_create_wal_key(InternalKey *rel_key_data, const RelFileLocato
8181
static inline void
8282
pg_tde_set_db_file_path(Oid dbOid, char *path)
8383
{
84-
join_path_components(path, pg_tde_get_data_dir(), psprintf(PG_TDE_MAP_FILENAME, dbOid));
84+
char *fname = psprintf(PG_TDE_MAP_FILENAME, dbOid);
85+
86+
join_path_components(path, pg_tde_get_data_dir(), fname);
87+
pfree(fname);
8588
}
8689

8790
extern void pg_tde_save_smgr_key(RelFileLocator rel, const InternalKey *key);

contrib/pg_tde/src/include/catalog/tde_keyring.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,5 +40,6 @@ extern void redo_key_provider_info(KeyringProviderRecordInFile *xlrec);
4040
extern void ParseKeyringJSONOptions(ProviderType provider_type,
4141
GenericKeyring *out_opts,
4242
char *in_buf, int buf_len);
43+
extern void free_keyring(GenericKeyring *keyring);
4344

4445
#endif /* TDE_KEYRING_H */

contrib/pg_tde/src/pg_tde_change_key_provider.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,7 @@ main(int argc, char *argv[])
249249
controlfile->state != DB_SHUTDOWNED_IN_RECOVERY)
250250
pg_fatal("cluster must be shut down");
251251

252+
pfree(controlfile);
252253
cptr = strcat(cptr, datadir);
253254
cptr = strcat(cptr, "/");
254255
cptr = strcat(cptr, PG_TDE_DATA_DIR);

src/bin/psql/common.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -790,6 +790,8 @@ StoreQueryTuple(const PGresult *result)
790790
{
791791
pg_log_warning("attempt to \\gset into specially treated variable \"%s\" ignored",
792792
varname);
793+
794+
free(varname);
793795
continue;
794796
}
795797

0 commit comments

Comments
 (0)