Skip to content

Tidy sanitizers' suppressions list and fix some mem leaks #438

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: TDE_REL_17_STABLE
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 47 additions & 9 deletions ci_scripts/suppressions/lsan.supp
Original file line number Diff line number Diff line change
@@ -1,14 +1,52 @@
# external submodule
leak:kmip.c

# the parser of command line arguments of the backend
leak:save_ps_display_args

# a bunch of not freed addrinfo sctructs, escaped values etc.
leak:initdb.c
leak:fe_memutils.c
leak:fe-exec.c
leak:fe-connect.c
leak:pqexpbuffer.c
leak:strdup
leak:preproc.y
leak:pgtypeslib/common.c
leak:ecpglib/memory.c
leak:kmip.c

# shlibs loaded with dlsym() never gets dlclose'd
# TODO: needs an investigation as it is down the call stack
# of exec_simple_query->PortalRun
leak:internal_load_library

# pg_config utility does not free configdata
leak:pg_config/pg_config.c

# A psprintf() result assigned to the global var pgdata_opt
leak:bin/pg_ctl/pg_ctl.c

# TODO: A couple of not freed char *. Whilst wit's trivially to fix
# meson complains with "maybe-uninitialized" on free()
leak:bin/psql/describe.c

# options parsing during the regressions start
leak:regression_main

# Static funcs in /bin/psql/startup.c are used
# during the start to parse options
leak:simple_action_list_append
leak:parse_psql_options

# A bunch of parameters, options and identifiers are not being freed.
#
# TODO?: ReceiveArchiveStreamChunk->CreateBackupStreamer down in the
# callstak creates a streamer that is never freed. It's being called in a loop
# but it looks like it is not a repetitive action and rather happens once per
# receiving CopyData message from server.
#
# bin/pg_basebackup/pg_basebackup.c
leak:BaseBackup

# `varname` doesn't get freed in the case of warning and `continue` in the loop
#
# bin/psql/common.c
leak:StoreQueryTuple

# does not free a bunch of parsed flags
leak:bin/pg_waldump/pg_waldump.c

#pg_dump
leak:getSchemaData
Expand Down
7 changes: 7 additions & 0 deletions contrib/pg_tde/src/access/pg_tde_tdemap.c
Original file line number Diff line number Diff line change
Expand Up @@ -1077,6 +1077,10 @@ pg_tde_fetch_wal_keys(XLogRecPtr start_lsn)

wal_rec = pg_tde_add_wal_key_to_cache(&stub_key, InvalidXLogRecPtr);

#ifdef FRONTEND
/* The backen frees it after copying to the cache. */
pfree(principal_key);
#endif
LWLockRelease(lock_pk);
CloseTransientFile(fd);
return wal_rec;
Expand Down Expand Up @@ -1107,6 +1111,9 @@ pg_tde_fetch_wal_keys(XLogRecPtr start_lsn)
return_wal_rec = wal_rec;
}
}
#ifdef FRONTEND
pfree(principal_key);
#endif
LWLockRelease(lock_pk);
CloseTransientFile(fd);

Expand Down
58 changes: 57 additions & 1 deletion contrib/pg_tde/src/catalog/tde_keyring.c
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,51 @@ check_provider_record(KeyringProviderRecord *provider_record)

#endif /* !FRONTEND */

void
free_keyring(GenericKeyring *keyring)
{
FileKeyring *file = (FileKeyring *) keyring;
VaultV2Keyring *vault = (VaultV2Keyring *) keyring;
KmipKeyring *kmip = (KmipKeyring *) keyring;

switch (keyring->type)
{
case FILE_KEY_PROVIDER:
if (file->file_name)
pfree(file->file_name);
break;
case VAULT_V2_KEY_PROVIDER:
if (vault->vault_ca_path)
pfree(vault->vault_ca_path);
if (vault->vault_mount_path)
pfree(vault->vault_mount_path);
if (vault->vault_token)
pfree(vault->vault_token);
if (vault->vault_token_path)
pfree(vault->vault_token_path);
if (vault->vault_url)
pfree(vault->vault_url);
break;
case KMIP_KEY_PROVIDER:
if (kmip->kmip_ca_path)
pfree(kmip->kmip_ca_path);
if (kmip->kmip_cert_path)
pfree(kmip->kmip_cert_path);
if (kmip->kmip_host)
pfree(kmip->kmip_host);
if (kmip->kmip_key_path)
pfree(kmip->kmip_key_path);
if (kmip->kmip_port)
pfree(kmip->kmip_port);
break;
default:
Assert(false);
break;
}

pfree(keyring);
}

void
write_key_provider_info(KeyringProviderRecordInFile *record, bool write_xlog)
{
Expand Down Expand Up @@ -682,6 +727,8 @@ simple_list_free(SimplePtrList *list)
pfree(cell);
cell = next;
}

pfree(list);
}
#endif /* FRONTEND */

Expand Down Expand Up @@ -818,6 +865,8 @@ load_file_keyring_provider_options(char *keyring_options)
ereport(WARNING,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("file path is missing in the keyring options"));

free_keyring((GenericKeyring *) file_keyring);
return NULL;
}

Expand Down Expand Up @@ -845,6 +894,8 @@ load_vaultV2_keyring_provider_options(char *keyring_options)
(vaultV2_keyring->vault_token_path != NULL && vaultV2_keyring->vault_token_path[0] != '\0') ? "" : " tokenPath",
(vaultV2_keyring->vault_url != NULL && vaultV2_keyring->vault_url[0] != '\0') ? "" : " url",
(vaultV2_keyring->vault_mount_path != NULL && vaultV2_keyring->vault_mount_path[0] != '\0') ? "" : " mountPath"));

free_keyring((GenericKeyring *) vaultV2_keyring);
return NULL;
}

Expand Down Expand Up @@ -878,6 +929,8 @@ load_kmip_keyring_provider_options(char *keyring_options)
(kmip_keyring->kmip_ca_path != NULL && kmip_keyring->kmip_ca_path[0] != '\0') ? "" : " caPath",
(kmip_keyring->kmip_cert_path != NULL && kmip_keyring->kmip_cert_path[0] != '\0') ? "" : " certPath",
(kmip_keyring->kmip_key_path != NULL && kmip_keyring->kmip_key_path[0] != '\0') ? "" : " keyPath"));

free_keyring((GenericKeyring *) kmip_keyring);
return NULL;
}

Expand Down Expand Up @@ -946,7 +999,10 @@ debug_print_kerying(GenericKeyring *keyring)
static inline void
get_keyring_infofile_path(char *resPath, Oid dbOid)
{
join_path_components(resPath, pg_tde_get_data_dir(), psprintf(PG_TDE_KEYRING_FILENAME, dbOid));
char *fname = psprintf(PG_TDE_KEYRING_FILENAME, dbOid);

join_path_components(resPath, pg_tde_get_data_dir(), fname);
pfree(fname);
}

static int
Expand Down
2 changes: 1 addition & 1 deletion contrib/pg_tde/src/catalog/tde_principal_key.c
Original file line number Diff line number Diff line change
Expand Up @@ -863,7 +863,7 @@ get_principal_key_from_keyring(Oid dbOid)
Assert(dbOid == principalKey->keyInfo.databaseId);

pfree(keyInfo);
pfree(keyring);
free_keyring(keyring);
pfree(principalKeyInfo);

return principalKey;
Expand Down
5 changes: 4 additions & 1 deletion contrib/pg_tde/src/include/access/pg_tde_tdemap.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,10 @@ extern void pg_tde_create_wal_key(InternalKey *rel_key_data, const RelFileLocato
static inline void
pg_tde_set_db_file_path(Oid dbOid, char *path)
{
join_path_components(path, pg_tde_get_data_dir(), psprintf(PG_TDE_MAP_FILENAME, dbOid));
char *fname = psprintf(PG_TDE_MAP_FILENAME, dbOid);

join_path_components(path, pg_tde_get_data_dir(), fname);
pfree(fname);
}

extern void pg_tde_save_smgr_key(RelFileLocator rel, const InternalKey *key);
Expand Down
1 change: 1 addition & 0 deletions contrib/pg_tde/src/include/catalog/tde_keyring.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,5 +40,6 @@ extern void redo_key_provider_info(KeyringProviderRecordInFile *xlrec);
extern void ParseKeyringJSONOptions(ProviderType provider_type,
GenericKeyring *out_opts,
char *in_buf, int buf_len);
extern void free_keyring(GenericKeyring *keyring);

#endif /* TDE_KEYRING_H */
1 change: 1 addition & 0 deletions contrib/pg_tde/src/keyring/keyring_vault.c
Original file line number Diff line number Diff line change
Expand Up @@ -438,5 +438,6 @@ json_resp_object_field_start(void *state, char *fname, bool isnull)
break;
}

pfree(fname);
return JSON_SUCCESS;
}
1 change: 1 addition & 0 deletions contrib/pg_tde/src/pg_tde_change_key_provider.c
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ main(int argc, char *argv[])
controlfile->state != DB_SHUTDOWNED_IN_RECOVERY)
pg_fatal("cluster must be shut down");

pfree(controlfile);
cptr = strcat(cptr, datadir);
cptr = strcat(cptr, "/");
cptr = strcat(cptr, PG_TDE_DATA_DIR);
Expand Down