[DTrace-devel] [PATCH] Fix various memory leaks
Eugene Loh
eugene.loh at oracle.com
Wed May 20 12:15:37 PDT 2020
Not an in-depth review, but I looked and poked around at it for a while
and it looks good.
On 05/18/2020 10:43 AM, Kris Van Hees wrote:
> Testing dtrace with valgrind identified a bunch of significant memory
> leaks. These have been fixed.
>
> There is still some loss of memory due to string elements in probe
> descriptions not consistently using strdup() to ensure that they can
> be freed when no longer needed, and also in the buffer handling in
> the lexer. These are left for future investigation and resolution.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
> cmd/dtrace.c | 7 +++-
> libdtrace/dt_dlibs.c | 95 ++++++++++++++++++++++++++++++++++----------
> libdtrace/dt_htab.c | 16 ++++----
> libdtrace/dt_htab.h | 6 ++-
> libdtrace/dt_ident.c | 3 +-
> libdtrace/dt_open.c | 4 ++
> libdtrace/dt_probe.c | 21 +++++++---
> libdtrace/dt_subr.c | 1 +
> 8 files changed, 115 insertions(+), 38 deletions(-)
>
> diff --git a/cmd/dtrace.c b/cmd/dtrace.c
> index 3dc49800..cef86ef2 100644
> --- a/cmd/dtrace.c
> +++ b/cmd/dtrace.c
> @@ -760,7 +760,7 @@ chew(const dtrace_probedata_t *data, void *arg)
> if (!g_flowindent) {
> if (!g_quiet) {
> size_t len = strlen(pd->fun) + strlen(pd->prb) + 2;
> - char *name = malloc(len);
> + char *name = alloca(len);
>
> if (name == NULL)
> fatal("failed to allocate memory for name");
> @@ -1562,5 +1562,10 @@ main(int argc, char *argv[])
> dtrace_proc_release(g_dtp, g_psv[i]);
>
> dtrace_close(g_dtp);
> +
> + free(g_argv);
> + free(g_cmdv);
> + free(g_psv);
> +
> return (g_status);
> }
> diff --git a/libdtrace/dt_dlibs.c b/libdtrace/dt_dlibs.c
> index 2b8fe21d..445654d7 100644
> --- a/libdtrace/dt_dlibs.c
> +++ b/libdtrace/dt_dlibs.c
> @@ -29,7 +29,7 @@ struct dt_bpf_reloc {
>
> typedef struct dt_bpf_func dt_bpf_func_t;
> struct dt_bpf_func {
> - const char *name;
> + char *name;
> int id;
> uint64_t offset;
> size_t size;
> @@ -46,7 +46,8 @@ static dtrace_attribute_t dt_bpf_attr = DT_ATTR_STABCMN;
> #define DT_BPF_SYMBOL_ID(name, type, id) \
> { __stringify(name), (type), DT_IDFLG_BPF, (id), \
> DT_ATTR_STABCMN, DT_VERS_2_0, }
> -static const dt_ident_t dt_bpf_symbols[] = {
> +
> +static const dt_ident_t dt_bpf_symbols[] = {
> /* BPF built-in functions */
> DT_BPF_SYMBOL(dt_predicate, DT_IDENT_FUNC),
> DT_BPF_SYMBOL(dt_program, DT_IDENT_FUNC),
> @@ -75,6 +76,43 @@ static const dt_ident_t dt_bpf_symbols[] = {
> #undef DT_BPF_SYMBOL
> #undef DT_BPF_SYMBOL_ID
>
> +static void
> +dt_idcook_none(dt_node_t *dnp, dt_ident_t *idp, int argc, dt_node_t *args)
> +{
> +}
> +
> +static void
> +dt_iddtor_bpf(dt_ident_t *idp)
> +{
> + dt_bpf_func_t *fp = idp->di_data;
> + dtrace_hdl_t *dtp = idp->di_iarg;
> +
> + if (fp == NULL)
> + return;
> +
> + if (fp->name != NULL)
> + free(fp->name);
> + if (fp->difo != NULL)
> + dt_difo_free(dtp, fp->difo);
> +
> + dt_free(dtp, fp);
> +}
> +
> +static size_t
> +dt_idsize_none(dt_ident_t *idp)
> +{
> + return 0;
> +}
> +
> +/*
> + * Identifier operations for entries in the identifier hash.
> + */
> +static const dt_idops_t dt_idops_bpf = {
> + dt_idcook_none,
> + dt_iddtor_bpf,
> + dt_idsize_none
> +};
> +
> /*
> * Perform initialization for the BPF function library handling.
> */
> @@ -113,7 +151,8 @@ dt_dlib_add_sym(dtrace_hdl_t *dtp, const char *name, int kind, void *data)
> dt_ident_t *idp;
>
> idp = dt_idhash_insert(dhp, name, kind, DT_IDFLG_BPF, DT_IDENT_UNDEF,
> - dt_bpf_attr, DT_VERS_2_0, NULL, NULL, 0);
> + dt_bpf_attr, DT_VERS_2_0, &dt_idops_bpf, dtp,
> + 0);
> dt_ident_set_data(idp, data);
>
> return idp;
> @@ -260,9 +299,9 @@ relcmp(const void *a, const void *b)
> * gcc BPF compiler not emitting symbol sizes), and then associate any listed
> * relocations with each function.
> */
> -static dt_bpf_func_t **
> +static int
> get_symbols(dtrace_hdl_t *dtp, Elf *elf, int syms_idx, int strs_idx,
> - int text_idx, int text_len, int relo_idx, int maps_idx, int *cnt)
> + int text_idx, int text_len, int relo_idx, int maps_idx)
> {
> int idx, fid, fun0;
> Elf_Scn *scn;
> @@ -292,7 +331,7 @@ get_symbols(dtrace_hdl_t *dtp, Elf *elf, int syms_idx, int strs_idx,
> * Allocate the symbol list.
> */
> symc = symtab->d_size / sizeof(GElf_Sym);
> - syms = calloc(symc, sizeof(dt_bpf_func_t *));
> + syms = dt_calloc(dtp, symc, sizeof(dt_bpf_func_t *));
> if (syms == NULL) {
> fprintf(stderr, "Failed to allocate symbol list\n");
> symc = 0;
> @@ -336,9 +375,14 @@ get_symbols(dtrace_hdl_t *dtp, Elf *elf, int syms_idx, int strs_idx,
> syms[idx] = fp;
>
> idp = dt_dlib_get_func(dtp, name);
> - if (idp != NULL)
> + if (idp != NULL) {
> + /*
> + * Nasty trick to get our idops in place.
> + */
> + dt_ident_morph(idp, idp->di_kind,
> + &dt_idops_bpf, dtp);
> dt_ident_set_data(idp, fp);
> - else {
> + } else {
> idp = dt_dlib_add_func(dtp, name, fp);
> if (idp == NULL) {
> fprintf(stderr,
> @@ -458,6 +502,7 @@ get_symbols(dtrace_hdl_t *dtp, Elf *elf, int syms_idx, int strs_idx,
> */
> if (fp->size % sizeof(struct bpf_insn)) {
> fprintf(stderr, "'%s' seems truncated\n", fp->name);
> + dt_free(dtp, dp);
> goto out;
> }
> if (*(uint64_t *)((char*)text->d_buf + fp->offset + fp->size -
> @@ -490,7 +535,7 @@ get_symbols(dtrace_hdl_t *dtp, Elf *elf, int syms_idx, int strs_idx,
> * DIFO. This will help us when we construct the actual DIFO relocs.
> */
> relc = data->d_size / sizeof(GElf_Rel);
> - rels = calloc(relc, sizeof(dt_bpf_reloc_t));
> + rels = dt_calloc(dtp, relc, sizeof(dt_bpf_reloc_t));
>
> for (idx = 0, rp = rels; idx < relc; idx++, rp++) {
> GElf_Rel rel;
> @@ -599,13 +644,27 @@ done:
> dt_strtab_destroy(stab);
> }
>
> + /*
> + * Clean up fake function symbols.
> + */
> + for (idx = 0; idx < symc; idx++) {
> + fp = syms[idx];
> + if (fp == NULL ||
> + fp->id != -1 || fp->offset > 0 || fp->size > 0)
> + continue;
> +
> + if (fp->name)
> + free(fp->name);
> +
> + dt_free(dtp, fp);
> + }
> +
> out:
> + dt_free(dtp, syms);
> dt_free(dtp, rels);
> dt_free(dtp, funcs);
>
> - *cnt = symc;
> -
> - return syms;
> + return symc == 0 ? -1 : 0;
> }
>
> static int
> @@ -624,8 +683,6 @@ readBPFFile(dtrace_hdl_t *dtp, const char *fn)
> Elf_Scn *scn;
> GElf_Ehdr ehdr;
> GElf_Shdr shdr;
> - dt_bpf_func_t **syms;
> - int symc;
>
> /*
> * Open the ELF object file and perform basic validation.
> @@ -718,12 +775,8 @@ readBPFFile(dtrace_hdl_t *dtp, const char *fn)
> break;
> }
>
> - syms = get_symbols(dtp, elf, syms_idx, strs_idx, text_idx, text_len,
> - relo_idx, maps_idx, &symc);
> - if (syms == NULL)
> - goto out;
> -
> - rc = 0;
> + rc = get_symbols(dtp, elf, syms_idx, strs_idx, text_idx, text_len,
> + relo_idx, maps_idx);
> goto out;
>
> err_elf:
> @@ -988,7 +1041,7 @@ dt_load_libs_dir(dtrace_hdl_t *dtp, const char *path)
>
> if (type == DT_DLIB_BPF) {
> readBPFFile(dtp, fname);
> - return 0;
> + continue;
> }
>
> if ((fp = fopen(fname, "r")) == NULL) {
> diff --git a/libdtrace/dt_htab.c b/libdtrace/dt_htab.c
> index 5342a518..e8e7c13b 100644
> --- a/libdtrace/dt_htab.c
> +++ b/libdtrace/dt_htab.c
> @@ -29,7 +29,7 @@
> #include <stdint.h>
> #include <stdlib.h>
>
> -#include "dt_htab.h"
> +#include "dt_impl.h"
>
> typedef struct dt_hbucket dt_hbucket_t;
> struct dt_hbucket {
> @@ -50,9 +50,9 @@ struct dt_htab {
> /*
> * Create a new (empty) hashtable.
> */
> -dt_htab_t *dt_htab_create(dt_htab_ops_t *ops)
> +dt_htab_t *dt_htab_create(dtrace_hdl_t *dtp, dt_htab_ops_t *ops)
> {
> - dt_htab_t *htab = malloc(sizeof(dt_htab_t));
> + dt_htab_t *htab = dt_alloc(dtp, sizeof(dt_htab_t));
>
> if (!htab)
> return NULL;
> @@ -62,9 +62,9 @@ dt_htab_t *dt_htab_create(dt_htab_ops_t *ops)
> htab->nbuckets = 0;
> htab->ops = ops;
>
> - htab->tab = calloc(htab->size, sizeof(dt_hbucket_t *));
> + htab->tab = dt_calloc(dtp, htab->size, sizeof(dt_hbucket_t *));
> if (!htab->tab) {
> - free(htab);
> + dt_free(dtp, htab);
> return NULL;
> }
>
> @@ -74,13 +74,13 @@ dt_htab_t *dt_htab_create(dt_htab_ops_t *ops)
> /*
> * Destroy a hashtable.
> */
> -void dt_htab_destroy(dt_htab_t *htab)
> +void dt_htab_destroy(dtrace_hdl_t *dtp, dt_htab_t *htab)
> {
> if (!htab)
> return;
>
> - free(htab->tab);
> - free(htab);
> + dt_free(dtp, htab->tab);
> + dt_free(dtp, htab);
> }
>
> /*
> diff --git a/libdtrace/dt_htab.h b/libdtrace/dt_htab.h
> index bf51a29c..00a480a8 100644
> --- a/libdtrace/dt_htab.h
> +++ b/libdtrace/dt_htab.h
> @@ -12,6 +12,8 @@
> extern "C" {
> #endif
>
> +struct dtrace_hdl;
> +
> typedef uint32_t (*htab_hval_fn)(const void *);
> typedef int (*htab_cmp_fn)(const void *, const void *);
> typedef void *(*htab_add_fn)(void *, void *);
> @@ -31,8 +33,8 @@ typedef struct dt_hentry {
>
> typedef struct dt_htab dt_htab_t;
>
> -extern dt_htab_t *dt_htab_create(dt_htab_ops_t *ops);
> -extern void dt_htab_detroy(dt_htab_t *htab);
> +extern dt_htab_t *dt_htab_create(struct dtrace_hdl *dtp, dt_htab_ops_t *ops);
> +extern void dt_htab_destroy(struct dtrace_hdl *dtp, dt_htab_t *htab);
> extern int dt_htab_insert(dt_htab_t *htab, void *entry);
> extern void *dt_htab_lookup(const dt_htab_t *htab, const void *entry);
> extern int dt_htab_delete(dt_htab_t *htab, void *entry);
> diff --git a/libdtrace/dt_ident.c b/libdtrace/dt_ident.c
> index 28c59158..e148821f 100644
> --- a/libdtrace/dt_ident.c
> +++ b/libdtrace/dt_ident.c
> @@ -946,7 +946,8 @@ void
> dt_ident_morph(dt_ident_t *idp, ushort_t kind,
> const dt_idops_t *ops, void *iarg)
> {
> - idp->di_ops->di_dtor(idp);
> + if (idp->di_ops)
> + idp->di_ops->di_dtor(idp);
> idp->di_kind = kind;
> idp->di_ops = ops;
> idp->di_iarg = iarg;
> diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> index 0209840f..2c57a8a3 100644
> --- a/libdtrace/dt_open.c
> +++ b/libdtrace/dt_open.c
> @@ -1144,6 +1144,8 @@ dtrace_close(dtrace_hdl_t *dtp)
> if (dtp == NULL)
> return;
>
> + dt_free(dtp, dtp->dt_conf.cpuids);
> +
> if (dtp->dt_procs != NULL)
> dt_proc_hash_destroy(dtp);
>
> @@ -1170,6 +1172,8 @@ dtrace_close(dtrace_hdl_t *dtp)
> dt_idhash_destroy(dtp->dt_globals);
> if (dtp->dt_tls != NULL)
> dt_idhash_destroy(dtp->dt_tls);
> + if (dtp->dt_bpfsyms != NULL)
> + dt_idhash_destroy(dtp->dt_bpfsyms);
>
> while ((dmp = dt_list_next(&dtp->dt_modlist)) != NULL)
> dt_module_destroy(dtp, dmp);
> diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
> index 823f0dd2..b6c79354 100644
> --- a/libdtrace/dt_probe.c
> +++ b/libdtrace/dt_probe.c
> @@ -1259,11 +1259,11 @@ dtrace_probe_iter(dtrace_hdl_t *dtp, const dtrace_probedesc_t *pdp,
> void
> dt_probe_init(dtrace_hdl_t *dtp)
> {
> - dtp->dt_byprv = dt_htab_create(&prv_htab_ops);
> - dtp->dt_bymod = dt_htab_create(&mod_htab_ops);
> - dtp->dt_byfun = dt_htab_create(&fun_htab_ops);
> - dtp->dt_byprb = dt_htab_create(&prb_htab_ops);
> - dtp->dt_byfqn = dt_htab_create(&fqn_htab_ops);
> + dtp->dt_byprv = dt_htab_create(dtp, &prv_htab_ops);
> + dtp->dt_bymod = dt_htab_create(dtp, &mod_htab_ops);
> + dtp->dt_byfun = dt_htab_create(dtp, &fun_htab_ops);
> + dtp->dt_byprb = dt_htab_create(dtp, &prb_htab_ops);
> + dtp->dt_byfqn = dt_htab_create(dtp, &fqn_htab_ops);
>
> dtp->dt_probes = NULL;
> dtp->dt_probes_sz = 0;
> @@ -1283,6 +1283,17 @@ dt_probe_fini(dtrace_hdl_t *dtp)
>
> dt_probe_destroy(prp);
> }
> +
> + dt_htab_destroy(dtp, dtp->dt_byprv);
> + dt_htab_destroy(dtp, dtp->dt_bymod);
> + dt_htab_destroy(dtp, dtp->dt_byfun);
> + dt_htab_destroy(dtp, dtp->dt_byprb);
> + dt_htab_destroy(dtp, dtp->dt_byfqn);
> +
> + dt_free(dtp, dtp->dt_probes);
> + dtp->dt_probes = NULL;
> + dtp->dt_probes_sz = 0;
> + dtp->dt_probe_id = 1;
> }
>
> void
> diff --git a/libdtrace/dt_subr.c b/libdtrace/dt_subr.c
> index 337827f4..3dcb92d8 100644
> --- a/libdtrace/dt_subr.c
> +++ b/libdtrace/dt_subr.c
> @@ -739,6 +739,7 @@ dt_difo_free(dtrace_hdl_t *dtp, dtrace_difo_t *dp)
> dt_free(dtp, dp->dtdo_buf);
> dt_free(dtp, dp->dtdo_strtab);
> dt_free(dtp, dp->dtdo_vartab);
> + dt_free(dtp, dp->dtdo_breltab);
> dt_free(dtp, dp->dtdo_kreltab);
> dt_free(dtp, dp->dtdo_ureltab);
> dt_free(dtp, dp->dtdo_xlmtab);
More information about the DTrace-devel
mailing list