[PATCH 00/17] Add automatic memory-leak detection to U-Boot tests
From: Simon Glass <sjg@chromium.org> This series adds built-in heap-leak detection that can be enabled with a single flag. It snapshots every in-use heap chunk before each test and reports any new allocations left behind afterwards, with full caller backtraces. Usage at the U-Boot command line: => ut -L dm dm_test_acpi_bgrt Test: acpi_bgrt: acpi.c Leak: 2 allocs 14a5c5c0 110 stdio_clone:230 <-vidconsole_post_probe:961 14a5c6d0 b0 map_to_sysmem:210 <-video_post_probe:823 Or via pytest: ./test/py/test.py -B sandbox --leak-check -k dm An interactive 'malloc leak' command is also provided for investigating leaks at the command line. The initial scan found 90 leaking test runs across 7 distinct bugs in SCMI, PMIC, SPI, PCI, ACPI and video subsystems, all fixed in this series. Simon Glass (17): kbuild: Use relative paths in generated .incbin directives blk: Return error from blk_get_devnum_by_uclass_idname() sandbox: Return -ENOMEM when os_map_file() fails vbe: Propagate probe errors from vbe_get_blk() malloc: Add heap-snapshot leak-checking functions test: Add memory leak checking option to ut command test: Show leaked allocations with ut -L test: Reset malloc backtrace collection before each test cmd: malloc: Add leak subcommand test: py: Add --leak-check option to pytest firmware: scmi: Fix memory leak in protocol list discovery power: pmic: Fix register leak in I2C PMIC emulator mtd: spi: Fix device name leak in sandbox SPI flash emulator pci: Fix PCI regions array leak on device removal test: dm: Fix memory leaks in ACPI DP tests dm: acpi: Fix memory leaks in ACPI item tracking and tests video: Fix map_to_sysmem() leak in show_splash() arch/sandbox/cpu/os.c | 2 +- boot/vbe_common.c | 11 +- cmd/Kconfig | 10 ++ cmd/blkmap.c | 8 +- cmd/malloc.c | 65 ++++++++ common/dlmalloc.c | 254 +++++++++++++++++++++++++++++ disk/part.c | 23 ++- doc/develop/malloc.rst | 68 +++++++- drivers/block/blk-uclass.c | 19 ++- drivers/block/blk_legacy.c | 3 +- drivers/core/acpi.c | 6 + drivers/firmware/scmi/base.c | 1 + drivers/mtd/spi/sandbox.c | 1 + drivers/pci/pci-uclass.c | 11 ++ drivers/power/pmic/i2c_pmic_emul.c | 11 ++ drivers/video/video-uclass.c | 1 + include/blk.h | 24 ++- include/malloc.h | 111 +++++++++++++ include/test/test.h | 2 + scripts/Makefile.lib | 11 +- test/boot/luks.c | 15 +- test/cmd/malloc.c | 52 ++++++ test/cmd_ut.c | 8 +- test/dm/acpi.c | 6 + test/dm/acpi_dp.c | 5 + test/py/conftest.py | 3 + test/py/tests/test_ut.py | 6 +- test/test-main.c | 24 +++ 28 files changed, 710 insertions(+), 51 deletions(-) -- 2.43.0 base-commit: cb53f60f48853713f398b86a12702304c82bdde7 branch: memb
From: Simon Glass <sjg@chromium.org> The generated .S files for fonts, images, splash screens, signer binaries and EFI apps use .incbin with the full prerequisite path. When building with O= this bakes an absolute path into the .S file. If the build directory is later used on a different machine (e.g. in a container), the assembler cannot find the source file. Use $(src)/$(notdir $<) instead of $< so the .incbin directive contains a source-relative path like drivers/video/images/u_boot.bmp rather than an absolute one. Add -Wa,-I$(srctree) to the assembler flags so the assembler can resolve these paths. The DTB and DTBO rules are left unchanged since their prerequisites are generated files in the build directory. Signed-off-by: Simon Glass <sjg@chromium.org> --- scripts/Makefile.lib | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index e2d0cb78bc9..8ccab5444cd 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -174,6 +174,7 @@ c_flags = -Wp,-MD,$(depfile) $(nostdinc) $(ubootinclude) \ $(basename_flags) $(modname_flags) a_flags = -Wp,-MD,$(depfile) $(nostdinc) $(ubootinclude) \ + -Wa,-I$(srctree) \ $(__a_flags) $(modkern_aflags) cpp_flags = -Wp,-MD,$(depfile) $(nostdinc) $(ubootinclude) \ @@ -494,7 +495,7 @@ cmd_S_ttf= \ echo '.balign 16'; \ echo '.global __ttf_$(*F)_begin'; \ echo '__ttf_$(*F)_begin:'; \ - echo '.incbin "$<" '; \ + echo '.incbin "$(src)/$(notdir $<)" '; \ echo '__ttf_$(*F)_end:'; \ echo '.global __ttf_$(*F)_end'; \ echo '.balign 16'; \ @@ -514,7 +515,7 @@ cmd_S_signer= \ echo '.balign 16'; \ echo '.global __signer_$(subst .,_,$(*F))_begin'; \ echo '__signer_$(subst .,_,$(*F))_begin:'; \ - echo '.incbin "$<" '; \ + echo '.incbin "$(src)/$(notdir $<)" '; \ echo '__signer_$(subst .,_,$(*F))_end:'; \ echo '.global __signer_$(subst .,_,$(*F))_end'; \ echo '.balign 16'; \ @@ -535,7 +536,7 @@ cmd_S_splash= \ echo '.balign 16'; \ echo '.global __splash_$(*F)_logo_begin'; \ echo '__splash_$(*F)_logo_begin:'; \ - echo '.incbin "$<" '; \ + echo '.incbin "$(src)/$(notdir $<)" '; \ echo '__splash_$(*F)_logo_end:'; \ echo '.global __splash_$(*F)_logo_end'; \ echo '.balign 16'; \ @@ -554,7 +555,7 @@ cmd_S_image= \ echo '.balign 16'; \ echo '.global __image_$(*F)_begin'; \ echo '__image_$(*F)_begin:'; \ - echo '.incbin "$<" '; \ + echo '.incbin "$(src)/$(notdir $<)" '; \ echo '__image_$(*F)_end:'; \ echo '.global __image_$(*F)_end'; \ echo '.balign 16'; \ @@ -594,7 +595,7 @@ cmd_S_efi= \ echo '.balign 16'; \ echo '.global __efi_$*_begin'; \ echo '__efi_$*_begin:'; \ - echo '.incbin "$<" '; \ + echo '.incbin "$(src)/$(notdir $<)" '; \ echo '__efi_$*_end:'; \ echo '.global __efi_$*_end'; \ echo '.balign 16'; \ -- 2.43.0
From: Simon Glass <sjg@chromium.org> blk_get_devnum_by_uclass_idname() returns a struct blk_desc pointer, discarding the error code from device_probe(). When probe fails due to out-of-memory or other errors, callers see NULL and report a generic 'device not found' instead of the actual cause. Change the function to return int and pass back the block device via a struct udevice ** parameter. This propagates the actual error from device_probe() through to callers. Rename the legacy version to blk_get_desc_by_uclass_idname() since it has no struct udevice to return, and use CONFIG_IS_ENABLED(BLK) in disk/part.c to select the right function. Signed-off-by: Simon Glass <sjg@chromium.org> --- cmd/blkmap.c | 8 ++++---- disk/part.c | 23 ++++++++++++++++++----- drivers/block/blk-uclass.c | 19 +++++++++++-------- drivers/block/blk_legacy.c | 3 ++- include/blk.h | 24 +++++++++++++++++++----- test/boot/luks.c | 15 +++++++++------ 6 files changed, 63 insertions(+), 29 deletions(-) diff --git a/cmd/blkmap.c b/cmd/blkmap.c index 65edec899e2..2dc516e2ca4 100644 --- a/cmd/blkmap.c +++ b/cmd/blkmap.c @@ -28,7 +28,7 @@ struct map_handler { static int do_blkmap_map_linear(struct map_ctx *ctx, int argc, char *const argv[]) { - struct blk_desc *lbd; + struct udevice *ldev; int err, ldevnum; lbaint_t lblknr; @@ -38,15 +38,15 @@ static int do_blkmap_map_linear(struct map_ctx *ctx, int argc, ldevnum = dectoul(argv[2], NULL); lblknr = dectoul(argv[3], NULL); - lbd = blk_get_devnum_by_uclass_idname(argv[1], ldevnum); - if (!lbd) { + err = blk_get_devnum_by_uclass_idname(argv[1], ldevnum, &ldev); + if (err) { printf("Found no device matching \"%s %d\"\n", argv[1], ldevnum); return CMD_RET_FAILURE; } err = blkmap_map_linear(ctx->dev, ctx->blknr, ctx->blkcnt, - lbd->bdev, lblknr); + ldev, lblknr); if (err) { printf("Unable to map \"%s %d\" at block 0x" LBAF ": %d\n", argv[1], ldevnum, ctx->blknr, err); diff --git a/disk/part.c b/disk/part.c index b6dd3598c0f..d5ebdb037a6 100644 --- a/disk/part.c +++ b/disk/part.c @@ -8,6 +8,7 @@ #include <blk.h> #include <command.h> +#include <dm.h> #include <env.h> #include <errno.h> #include <log.h> @@ -124,11 +125,23 @@ static struct blk_desc *get_dev_hwpart(const char *ifname, int dev, int hwpart) if (!blk_enabled()) return NULL; - desc = blk_get_devnum_by_uclass_idname(ifname, dev); - if (!desc) { - debug("%s: No device for iface '%s', dev %d\n", __func__, - ifname, dev); - return NULL; + if (CONFIG_IS_ENABLED(BLK)) { + struct udevice *blk; + + ret = blk_get_devnum_by_uclass_idname(ifname, dev, &blk); + if (ret) { + log_debug("No device for iface '%s', dev %d: err=%d\n", + ifname, dev, ret); + return NULL; + } + desc = dev_get_uclass_plat(blk); + } else { + desc = blk_get_desc_by_uclass_idname(ifname, dev); + if (!desc) { + log_debug("No device for iface '%s', dev %d\n", + ifname, dev); + return NULL; + } } ret = blk_dselect_hwpart(desc, hwpart); if (ret) { diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c index a6ef34c2d5f..9c9169d6ac8 100644 --- a/drivers/block/blk-uclass.c +++ b/drivers/block/blk-uclass.c @@ -107,7 +107,8 @@ struct blk_desc *blk_get_devnum_by_uclass_id(enum uclass_id uclass_id, int devnu * name in a local table. This gives us an interface type which we can match * against the uclass of the block device's parent. */ -struct blk_desc *blk_get_devnum_by_uclass_idname(const char *uclass_idname, int devnum) +int blk_get_devnum_by_uclass_idname(const char *uclass_idname, int devnum, + struct udevice **blkp) { enum uclass_id uclass_id; enum uclass_id type; @@ -119,18 +120,18 @@ struct blk_desc *blk_get_devnum_by_uclass_idname(const char *uclass_idname, int if (type == UCLASS_INVALID) { debug("%s: Unknown interface type '%s'\n", __func__, uclass_idname); - return NULL; + return -ENODEV; } uclass_id = conv_uclass_id(type); if (uclass_id == UCLASS_INVALID) { debug("%s: Unknown uclass for interface type'\n", blk_get_uclass_name(type)); - return NULL; + return -ENODEV; } ret = uclass_get(UCLASS_BLK, &uc); if (ret) - return NULL; + return ret; uclass_foreach_dev(dev, uc) { struct blk_desc *desc = dev_get_uclass_plat(dev); @@ -146,15 +147,17 @@ struct blk_desc *blk_get_devnum_by_uclass_idname(const char *uclass_idname, int continue; } - if (device_probe(dev)) - return NULL; + ret = device_probe(dev); + if (ret) + return ret; debug("%s: Device desc %p\n", __func__, desc); - return desc; + *blkp = dev; + return 0; } debug("%s: No device found\n", __func__); - return NULL; + return -ENXIO; } /** diff --git a/drivers/block/blk_legacy.c b/drivers/block/blk_legacy.c index f36932183d1..28e40d23c71 100644 --- a/drivers/block/blk_legacy.c +++ b/drivers/block/blk_legacy.c @@ -199,7 +199,8 @@ int blk_dselect_hwpart(struct blk_desc *desc, int hwpart) return 0; } -struct blk_desc *blk_get_devnum_by_uclass_idname(const char *uclass_idname, int devnum) +struct blk_desc *blk_get_desc_by_uclass_idname(const char *uclass_idname, + int devnum) { struct blk_driver *drv = blk_driver_lookup_typename(uclass_idname); struct blk_desc *desc; diff --git a/include/blk.h b/include/blk.h index 877876cb622..14ab77f7c51 100644 --- a/include/blk.h +++ b/include/blk.h @@ -642,15 +642,29 @@ struct blk_desc *blk_get_devnum_by_uclass_id(enum uclass_id uclass_id, int devnu /** * blk_get_devnum_by_uclass_idname() - Get block device by type name and number * - * This looks up the block device type based on @uclass_idname, then calls - * blk_get_devnum_by_uclass_id(). + * This looks up the block device type based on @uclass_idname, then probes + * the device. + * + * @uclass_idname: Block device type name (e.g. "mmc") + * @devnum: Device number + * @blkp: Returns the block device + * Return: 0 if OK, -ENODEV if the uclass is not known, -ENXIO if the + * device is not found, or other -ve on probe error + */ +int blk_get_devnum_by_uclass_idname(const char *uclass_idname, int devnum, + struct udevice **blkp); + +/** + * blk_get_desc_by_uclass_idname() - Get block descriptor by type name/number + * + * Legacy version for non-DM block devices. * * @uclass_idname: Block device type name * @devnum: Device number - * Return: point to block device descriptor, or NULL if not found + * Return: pointer to block device descriptor, or NULL if not found */ -struct blk_desc *blk_get_devnum_by_uclass_idname(const char *uclass_idname, - int devnum); +struct blk_desc *blk_get_desc_by_uclass_idname(const char *uclass_idname, + int devnum); /** * blk_dselect_hwpart() - select a hardware partition diff --git a/test/boot/luks.c b/test/boot/luks.c index 339c7d7fc94..f6d0d951fee 100644 --- a/test/boot/luks.c +++ b/test/boot/luks.c @@ -220,6 +220,7 @@ BOOTSTD_TEST(bootstd_test_luks2_info, UTF_DM | UTF_SCAN_FDT | UTF_CONSOLE); static int bootstd_test_luks_unlock(struct unit_test_state *uts) { struct blk_desc *desc; + struct udevice *blk; struct udevice *mmc; loff_t file_size; @@ -242,8 +243,8 @@ static int bootstd_test_luks_unlock(struct unit_test_state *uts) ut_assert_console_end(); /* Verify that a file can be read from the decrypted filesystem */ - desc = blk_get_devnum_by_uclass_idname("blkmap", 0); - ut_assertnonnull(desc); + ut_assertok(blk_get_devnum_by_uclass_idname("blkmap", 0, &blk)); + desc = dev_get_uclass_plat(blk); ut_assertok(fs_set_blk_dev_with_part(desc, 0)); ut_assertok(fs_size("/bin/bash", &file_size)); @@ -262,6 +263,7 @@ static int bootstd_test_luks2_unlock(struct unit_test_state *uts) { struct disk_partition info; struct blk_desc *desc; + struct udevice *blk; struct udevice *mmc; u8 master_key[512]; loff_t file_size; @@ -284,8 +286,8 @@ static int bootstd_test_luks2_unlock(struct unit_test_state *uts) ut_assert_console_end(); /* Verify that a file can be read from the decrypted filesystem */ - desc = blk_get_devnum_by_uclass_idname("blkmap", 0); - ut_assertnonnull(desc); + ut_assertok(blk_get_devnum_by_uclass_idname("blkmap", 0, &blk)); + desc = dev_get_uclass_plat(blk); ut_assertok(fs_set_blk_dev_with_part(desc, 0)); ut_assertok(fs_size("/bin/bash", &file_size)); @@ -312,6 +314,7 @@ static int setup_mmc14(struct unit_test_state *uts, struct udevice **mmcp) static int bootstd_test_luks2_unlock_prederived(struct unit_test_state *uts) { struct blk_desc *desc; + struct udevice *blk; struct udevice *mmc; loff_t file_size; @@ -333,8 +336,8 @@ static int bootstd_test_luks2_unlock_prederived(struct unit_test_state *uts) ut_assert_console_end(); /* Verify that a file can be read from the decrypted filesystem */ - desc = blk_get_devnum_by_uclass_idname("blkmap", 0); - ut_assertnonnull(desc); + ut_assertok(blk_get_devnum_by_uclass_idname("blkmap", 0, &blk)); + desc = dev_get_uclass_plat(blk); ut_assertok(fs_set_blk_dev_with_part(desc, 0)); ut_assertok(fs_size("/bin/bash", &file_size)); -- 2.43.0
From: Simon Glass <sjg@chromium.org> os_map_file() returns -EPERM for all mmap() failures regardless of the actual errno. When mmap() fails with ENOMEM, return -ENOMEM so that callers can distinguish out-of-memory from other errors. Signed-off-by: Simon Glass <sjg@chromium.org> --- arch/sandbox/cpu/os.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c index 142b685e031..c1381775b1b 100644 --- a/arch/sandbox/cpu/os.c +++ b/arch/sandbox/cpu/os.c @@ -299,7 +299,7 @@ int os_map_file(const char *pathname, int os_flags, void **bufp, int *sizep) ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, ifd, 0); if (ptr == MAP_FAILED) { printf("Can't map file '%s': %s\n", pathname, strerror(errno)); - ret = -EPERM; + ret = errno == ENOMEM ? -ENOMEM : -EPERM; goto out; } -- 2.43.0
From: Simon Glass <sjg@chromium.org> vbe_get_blk() uses the updated blk_get_devnum_by_uclass_idname() which now returns the actual error from device_probe(). This replaces the generic -ENXIO that was returned when the block device could not be found, making out-of-memory and other probe failures visible to the caller. Signed-off-by: Simon Glass <sjg@chromium.org> --- boot/vbe_common.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/boot/vbe_common.c b/boot/vbe_common.c index f9368a6a9ab..dbbbac3e96a 100644 --- a/boot/vbe_common.c +++ b/boot/vbe_common.c @@ -23,10 +23,11 @@ binman_sym_declare(ulong, u_boot_spl_bss_pad, size); int vbe_get_blk(const char *storage, struct udevice **blkp) { - struct blk_desc *desc; + struct udevice *blk; char devname[16]; const char *end; int devnum; + int ret; /* First figure out the block device */ log_debug("storage=%s\n", storage); @@ -38,10 +39,10 @@ int vbe_get_blk(const char *storage, struct udevice **blkp) strlcpy(devname, storage, end - storage + 1); log_debug("dev=%s, %x\n", devname, devnum); - desc = blk_get_dev(devname, devnum); - if (!desc) - return log_msg_ret("get", -ENXIO); - *blkp = desc->bdev; + ret = blk_get_devnum_by_uclass_idname(devname, devnum, &blk); + if (ret) + return log_msg_ret("get", ret); + *blkp = blk; return 0; } -- 2.43.0
From: Simon Glass <sjg@chromium.org> Add malloc_leak_check_start(), malloc_leak_check_end(), malloc_leak_check_count() and malloc_leak_check_free() which take a snapshot of every in-use heap chunk and later report any new allocations with their address, size and caller backtrace. The snapshot is stored using os_malloc() so it does not perturb the heap being measured. Signed-off-by: Simon Glass <sjg@chromium.org> --- common/dlmalloc.c | 230 ++++++++++++++++++++++++++++++++++++++++++++++ include/malloc.h | 81 ++++++++++++++++ 2 files changed, 311 insertions(+) diff --git a/common/dlmalloc.c b/common/dlmalloc.c index 2bc5f63c0a8..e61f565bbab 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -7634,7 +7634,237 @@ int malloc_dump_to_file(const char *fname) return 0; } +#endif /* IS_ENABLED(CONFIG_SANDBOX) */ + +/** + * count_used_chunks() - Count the number of in-use heap chunks + * + * Walk the entire dlmalloc heap and count chunks that are currently in use. + * + * Return: number of used chunks + */ +static int count_used_chunks(void) +{ + mchunkptr q; + msegmentptr s; + int count = 0; + + if (!is_initialized(gm)) + return 0; + + for (s = &gm->seg; s; s = s->next) { + q = align_as_chunk(s->base); + while (segment_holds(s, q) && + q != gm->top && q->head != FENCEPOST_HEAD) { + if (is_inuse(q)) + count++; + q = next_chunk(q); + } + } + + return count; +} + +size_t malloc_mcheck_hdr_size(void) +{ +#if CONFIG_IS_ENABLED(MCHECK_HEAP_PROTECTION) + return sizeof(struct mcheck_hdr); +#else + return 0; +#endif +} + +size_t malloc_chunk_size(void *ptr) +{ + void *mem; + mchunkptr p; + + mem = (char *)ptr - malloc_mcheck_hdr_size(); + p = mem2chunk(mem); + + return chunksize(p); +} + +int malloc_leak_check_start(struct malloc_leak_snap *snap) +{ + mchunkptr q; + msegmentptr s; + int i; + + if (!is_initialized(gm)) + return -ENOENT; + + snap->count = count_used_chunks(); + + /* + * On sandbox, allocate from the host OS so the snapshot does not + * disturb the U-Boot heap. On other platforms, use the heap itself + * but reserve one extra slot for the snapshot's own chunk. + */ +#if IS_ENABLED(CONFIG_SANDBOX) + snap->addr = os_malloc(snap->count * sizeof(ulong)); +#else + snap->addr = malloc((snap->count + 1) * sizeof(ulong)); #endif + if (!snap->addr) + return -ENOMEM; + + i = 0; + for (s = &gm->seg; s; s = s->next) { + q = align_as_chunk(s->base); + while (segment_holds(s, q) && + q != gm->top && q->head != FENCEPOST_HEAD) { + if (is_inuse(q)) + snap->addr[i++] = (ulong)chunk2mem(q); + q = next_chunk(q); + } + } + snap->count = i; + + return 0; +} + +/** + * snap_has_addr() - Check whether an address is in the snapshot + * + * The addresses are stored in ascending heap order, so use binary search. + * + * @snap: Snapshot taken earlier + * @addr: Address to look for + * Return: true if found + */ +static bool snap_has_addr(struct malloc_leak_snap *snap, ulong addr) +{ + int lo = 0, hi = snap->count - 1; + + while (lo <= hi) { + int mid = lo + (hi - lo) / 2; + + if (snap->addr[mid] == addr) + return true; + if (snap->addr[mid] < addr) + lo = mid + 1; + else + hi = mid - 1; + } + + return false; +} + +/** + * count_new_allocs() - Count heap allocations not present in the snapshot + * + * @snap: Snapshot taken earlier + * Return: number of new allocations found + */ +static int count_new_allocs(struct malloc_leak_snap *snap) +{ + msegmentptr s; + int leaks = 0; + mchunkptr q; + + for (s = &gm->seg; s; s = s->next) { + q = align_as_chunk(s->base); + while (segment_holds(s, q) && + q != gm->top && q->head != FENCEPOST_HEAD) { + if (is_inuse(q) && + !snap_has_addr(snap, (ulong)chunk2mem(q))) + leaks++; + q = next_chunk(q); + } + } + + return leaks; +} + +/** + * print_new_allocs() - Print heap allocations not present in the snapshot + * + * @snap: Snapshot taken earlier + */ +static void print_new_allocs(struct malloc_leak_snap *snap) +{ + msegmentptr s; + mchunkptr q; + + for (s = &gm->seg; s; s = s->next) { + q = align_as_chunk(s->base); + while (segment_holds(s, q) && + q != gm->top && q->head != FENCEPOST_HEAD) { + if (is_inuse(q) && + !snap_has_addr(snap, (ulong)chunk2mem(q))) { + const char *caller = ""; + void *mem = chunk2mem(q); + size_t sz = chunksize(q); + +#if CONFIG_IS_ENABLED(MCHECK_HEAP_PROTECTION) + /* + * Read the caller directly from the mcheck + * header at the start of the chunk rather + * than searching the registry, which may + * have overflowed. Validate the canary first + * to avoid printing garbage from chunks + * allocated without mcheck (e.g. when mcheck + * was temporarily disabled). + */ + struct mcheck_hdr *hdr = mem; + int j; + + for (j = 0; j < CANARY_DEPTH; j++) + if (hdr->canary.elems[j] != MAGICWORD) + break; + if (j == CANARY_DEPTH && hdr->caller[0]) + caller = hdr->caller; +#endif + printf(" %lx %zx %s\n", (ulong)mem, sz, + caller); + } + q = next_chunk(q); + } + } +} + +int malloc_leak_check_end(struct malloc_leak_snap *snap) +{ + int leaks; + + if (!is_initialized(gm) || !snap->addr) + return -ENOENT; + + leaks = count_new_allocs(snap); + if (leaks) + print_new_allocs(snap); + + +#if IS_ENABLED(CONFIG_SANDBOX) + os_free(snap->addr); +#else + free(snap->addr); +#endif + snap->addr = NULL; + snap->count = 0; + + return leaks; +} + +int malloc_leak_check_count(struct malloc_leak_snap *snap) +{ + if (!is_initialized(gm) || !snap->addr) + return -ENOENT; + + return count_new_allocs(snap); +} + +void malloc_leak_check_free(struct malloc_leak_snap *snap) +{ +#if IS_ENABLED(CONFIG_SANDBOX) + os_free(snap->addr); +#else + free(snap->addr); +#endif + snap->addr = NULL; + snap->count = 0; +} int initf_malloc(void) { diff --git a/include/malloc.h b/include/malloc.h index 610289f3a6c..ded5520ca23 100644 --- a/include/malloc.h +++ b/include/malloc.h @@ -146,6 +146,21 @@ struct mallinfo { #endif /* HAVE_USR_INCLUDE_MALLOC_H */ #endif /* !NO_MALLINFO */ +/** + * struct malloc_leak_snap - Snapshot of heap allocations for leak checking + * + * Used by malloc_leak_check_start/end to record the set of in-use chunk + * addresses before a test, so that new (leaked) allocations can be identified + * afterwards. + * + * @addr: Array of chunk addresses + * @count: Number of entries in @addr + */ +struct malloc_leak_snap { + unsigned long *addr; + int count; +}; + /* malloc(size_t n) Returns a pointer to a newly allocated chunk of at least n bytes, or @@ -818,6 +833,72 @@ void malloc_backtrace_skip(bool skip); static inline void malloc_backtrace_skip(bool skip) {} #endif +/** + * malloc_chunk_size() - Return the dlmalloc chunk size for an allocation + * + * Given a pointer returned by malloc(), return the size of the underlying + * dlmalloc chunk. This is the size shown in heap dumps and leak reports. + * + * @ptr: Pointer returned by malloc/calloc/realloc + * Return: chunk size in bytes + */ +size_t malloc_chunk_size(void *ptr); + +/** + * malloc_mcheck_hdr_size() - Return the size of the mcheck header + * + * When CONFIG_MCHECK_HEAP_PROTECTION is enabled, each allocation has a + * header placed before the returned pointer. This returns its size, or + * 0 when mcheck is disabled. This is useful for computing the chunk + * address from a malloc pointer (chunk_addr = ptr - hdr_size). + * + * Return: size of struct mcheck_hdr, or 0 + */ +size_t malloc_mcheck_hdr_size(void); + +/** + * malloc_leak_check_start() - Record current heap allocations + * + * Walk the heap and save the address of every in-use chunk. This snapshot + * is later compared by malloc_leak_check_end() to find new allocations. + * Storage is allocated with os_malloc() so it does not perturb the heap. + * + * @snap: Snapshot structure to fill in + * Return: 0 on success, -ENOENT if heap not initialised, -ENOMEM on failure + */ +int malloc_leak_check_start(struct malloc_leak_snap *snap); + +/** + * malloc_leak_check_end() - Report allocations not present in the snapshot + * + * Walk the heap and print every in-use chunk whose address was not recorded + * in the snapshot taken by malloc_leak_check_start(). Each leaked allocation + * is printed with its address, size, and caller (when mcheck is enabled). + * Frees the snapshot storage afterwards. + * + * @snap: Snapshot taken earlier with malloc_leak_check_start() + * Return: number of leaked allocations, or -ve on error + */ +int malloc_leak_check_end(struct malloc_leak_snap *snap); + +/** + * malloc_leak_check_count() - Count allocations not present in the snapshot + * + * Like malloc_leak_check_end() but only counts, without printing or freeing + * the snapshot. + * + * @snap: Snapshot taken earlier with malloc_leak_check_start() + * Return: number of leaked allocations, or -ve on error + */ +int malloc_leak_check_count(struct malloc_leak_snap *snap); + +/** + * malloc_leak_check_free() - Free a snapshot without checking + * + * @snap: Snapshot taken earlier with malloc_leak_check_start() + */ +void malloc_leak_check_free(struct malloc_leak_snap *snap); + /** * mem_malloc_init() - Initialize the malloc() heap * -- 2.43.0
From: Simon Glass <sjg@chromium.org> Add a -L flag to the ut command that checks for memory leaks around each test. When enabled, mallinfo() is called at the start and end of ut_run_test() and any difference in allocated bytes is reported. This is useful for finding memory leaks in driver model and other subsystems that should be fully cleaned up between tests. Usage: ut -L dm [test_name] Signed-off-by: Simon Glass <sjg@chromium.org> --- doc/develop/malloc.rst | 22 +++++++++++++++++++++- include/test/test.h | 2 ++ test/cmd_ut.c | 8 +++++++- test/test-main.c | 15 +++++++++++++++ 4 files changed, 45 insertions(+), 2 deletions(-) diff --git a/doc/develop/malloc.rst b/doc/develop/malloc.rst index 7e46c05dfde..74acc3220af 100644 --- a/doc/develop/malloc.rst +++ b/doc/develop/malloc.rst @@ -500,9 +500,29 @@ by checking ``malloc_get_info()`` before and after:: assert(before.malloc_count == after.malloc_count); assert(before.in_use_bytes == after.in_use_bytes); +**Automatic leak checking with ut -L** + +The ``ut`` command accepts a ``-L`` flag that checks for memory leaks around +each test. It takes a ``mallinfo()`` snapshot at the start of ``ut_run_test()`` +and compares it at the end, after both ``test_pre_run()`` and +``test_post_run()`` have completed:: + + => ut -L dm dm_test_acpi_bgrt + Test: acpi_bgrt: acpi.c + Leak: 448 bytes: acpi_bgrt + ... + +When using uman, pass ``--leak-check``:: + + $ um t --leak-check dm_test_acpi_bgrt + +This makes it easy to scan an entire test suite for leaks:: + + $ um t --leak-check -V dm + **Practical workflow** -1. Add ``ut_check_delta()`` assertions to your test to detect leaks +1. Run ``um t --leak-check -V dm`` (or another suite) to find leaky tests 2. When a leak is detected, add ``malloc_dump_to_file()`` calls before and after the leaking operation 3. Run the test and compare the dump files to identify leaked allocations diff --git a/include/test/test.h b/include/test/test.h index b81cab4d7a4..7d28948eb5f 100644 --- a/include/test/test.h +++ b/include/test/test.h @@ -102,6 +102,7 @@ struct ut_arg { * @arg_error: Set if ut_str/int/bool() detects a type mismatch * @keep_record: Preserve console recording when ut_fail() is called * @emit_result: Emit result line after each test completes + * @leak_check: Check for memory leaks around each test * @video_ctx: Vidconsole context for test message display (allocated on use) * @video_save: Saved framebuffer region for video tests * @priv: Private data for tests to use as needed @@ -139,6 +140,7 @@ struct unit_test_state { bool arg_error; bool keep_record; bool emit_result; + bool leak_check; void *video_ctx; struct abuf video_save; char priv[UT_PRIV_SIZE]; diff --git a/test/cmd_ut.c b/test/cmd_ut.c index f8ff02bdbc7..7de10b32065 100644 --- a/test/cmd_ut.c +++ b/test/cmd_ut.c @@ -258,6 +258,7 @@ static int do_ut(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) bool force_run = false; bool keep_record = false; bool emit_result = false; + bool leak_check = false; int runs_per_text = 1; int workers = 0, worker_id = 0; struct suite *ste; @@ -283,6 +284,9 @@ static int do_ut(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) case 'm': force_run = true; break; + case 'L': + leak_check = true; + break; case 'I': test_insert = str + 1; if (!strchr(test_insert, ':')) @@ -316,6 +320,7 @@ next_arg: ut_init_state(&uts); uts.keep_record = keep_record; uts.emit_result = emit_result; + uts.leak_check = leak_check; uts.workers = workers; uts.worker_id = worker_id; name = argv[0]; @@ -363,9 +368,10 @@ next_arg: } U_BOOT_LONGHELP(ut, - "[-Efmrs] [-R] [-I<n>:<one_test>] [-P<n>:<w>] <suite> [<test> [<args>...]]\n" + "[-ELfmrs] [-R] [-I<n>:<one_test>] [-P<n>:<w>] <suite> [<test> [<args>...]]\n" " - run unit tests\n" " -E Emit result line after each test\n" + " -L Check for memory leaks around each test\n" " -r<runs> Number of times to run each test\n" " -f/-m Force 'manual' tests to run as well\n" " -I Test to run after <n> other tests have run\n" diff --git a/test/test-main.c b/test/test-main.c index 77223cfbcb7..be13084ed92 100644 --- a/test/test-main.c +++ b/test/test-main.c @@ -631,6 +631,7 @@ static int ut_run_test(struct unit_test_state *uts, struct unit_test *test, { const char *fname = strrchr(test->file, '/') + 1; const char *note = ""; + struct mallinfo leak_before; int old_fail_count; int ret; @@ -638,6 +639,9 @@ static int ut_run_test(struct unit_test_state *uts, struct unit_test *test, note = " (flat tree)"; printf("Test: %s: %s%s\n", test_name, fname, note); + if (uts->leak_check) + leak_before = mallinfo(); + /* Allow access to test state from drivers */ ut_set_state(uts); @@ -659,6 +663,17 @@ static int ut_run_test(struct unit_test_state *uts, struct unit_test *test, ut_set_state(NULL); + if (uts->leak_check) { + struct mallinfo leak_after; + int diff; + + leak_after = mallinfo(); + diff = leak_after.uordblks - leak_before.uordblks; + if (diff) + printf("Leak: %d bytes: %s%s\n", diff, test_name, + note); + } + if (uts->emit_result) { bool passed = uts->cur.fail_count == old_fail_count; -- 2.43.0
From: Simon Glass <sjg@chromium.org> Use the new malloc_leak_check functions to show the address, size and caller backtrace of each leaked allocation, rather than just the total byte count. Example output: Test: acpi_bgrt: acpi.c Leak: 2 allocs 14a5c5c0 110 stdio_clone:230 <-stdio_register_dev:244 14a5c6d0 b0 map_to_sysmem:210 <-video_post_probe:823 Signed-off-by: Simon Glass <sjg@chromium.org> --- doc/develop/malloc.rst | 30 +++++++++++++++++------------- test/test-main.c | 21 +++++++++++---------- 2 files changed, 28 insertions(+), 23 deletions(-) diff --git a/doc/develop/malloc.rst b/doc/develop/malloc.rst index 74acc3220af..6bb17dca7b1 100644 --- a/doc/develop/malloc.rst +++ b/doc/develop/malloc.rst @@ -503,14 +503,21 @@ by checking ``malloc_get_info()`` before and after:: **Automatic leak checking with ut -L** The ``ut`` command accepts a ``-L`` flag that checks for memory leaks around -each test. It takes a ``mallinfo()`` snapshot at the start of ``ut_run_test()`` -and compares it at the end, after both ``test_pre_run()`` and -``test_post_run()`` have completed:: +each test. It snapshots every in-use heap chunk at the start of +``ut_run_test()`` and compares after both ``test_pre_run()`` and +``test_post_run()`` have completed. Any new allocations are reported with +their address, size and caller backtrace:: - => ut -L dm dm_test_acpi_bgrt + => ut -LE dm dm_test_acpi_bgrt Test: acpi_bgrt: acpi.c - Leak: 448 bytes: acpi_bgrt - ... + Leak: 2 allocs + 14a5c5c0 110 stdio_clone:230 <-stdio_register_dev:244 <-vidconsole_post_probe:961 + 14a5c6d0 b0 map_to_sysmem:210 <-video_post_probe:823 <-device_probe:589 + Result: PASS: acpi_bgrt: acpi.c + +The snapshot is stored using ``os_malloc()`` so it does not affect the +heap being measured. Caller backtraces are available when +``CONFIG_MCHECK_HEAP_PROTECTION`` is enabled (the default for sandbox). When using uman, pass ``--leak-check``:: @@ -523,13 +530,10 @@ This makes it easy to scan an entire test suite for leaks:: **Practical workflow** 1. Run ``um t --leak-check -V dm`` (or another suite) to find leaky tests -2. When a leak is detected, add ``malloc_dump_to_file()`` calls before and - after the leaking operation -3. Run the test and compare the dump files to identify leaked allocations -4. Use the caller backtrace in the dump to find the allocation site -5. If more detail is needed, enable ``malloc_log_start()`` to trace all - allocations during the operation -6. Fix the leak and verify the test passes +2. Use the caller backtrace in the ``-L`` output to find the allocation site +3. If more detail is needed, add ``malloc_dump_to_file()`` calls or enable + ``malloc_log_start()`` to trace all allocations during the operation +4. Fix the leak and verify the test passes **Dumping heap state on exit** diff --git a/test/test-main.c b/test/test-main.c index be13084ed92..09458fa91da 100644 --- a/test/test-main.c +++ b/test/test-main.c @@ -631,7 +631,7 @@ static int ut_run_test(struct unit_test_state *uts, struct unit_test *test, { const char *fname = strrchr(test->file, '/') + 1; const char *note = ""; - struct mallinfo leak_before; + struct malloc_leak_snap leak_snap = {}; int old_fail_count; int ret; @@ -640,7 +640,7 @@ static int ut_run_test(struct unit_test_state *uts, struct unit_test *test, printf("Test: %s: %s%s\n", test_name, fname, note); if (uts->leak_check) - leak_before = mallinfo(); + malloc_leak_check_start(&leak_snap); /* Allow access to test state from drivers */ ut_set_state(uts); @@ -664,14 +664,15 @@ static int ut_run_test(struct unit_test_state *uts, struct unit_test *test, ut_set_state(NULL); if (uts->leak_check) { - struct mallinfo leak_after; - int diff; - - leak_after = mallinfo(); - diff = leak_after.uordblks - leak_before.uordblks; - if (diff) - printf("Leak: %d bytes: %s%s\n", diff, test_name, - note); + int leaks; + + leaks = malloc_leak_check_count(&leak_snap); + if (leaks > 0) { + printf("Leak: %d allocs\n", leaks); + malloc_leak_check_end(&leak_snap); + } else { + malloc_leak_check_free(&leak_snap); + } } if (uts->emit_result) { -- 2.43.0
From: Simon Glass <sjg@chromium.org> The stack-protector test and __stack_chk_fail() call malloc_backtrace_skip(true) to prevent crashes when collecting backtraces from a corrupted stack. However, they never reset it, so all subsequent allocations in a long-running session (e.g. pytest) have empty caller backtraces. Add malloc_backtrace_unbusy() to clear the reentrant guard which can also get stuck if backtrace collection crashes. Reset both flags at the start of each test in test_pre_run() Signed-off-by: Simon Glass <sjg@chromium.org> --- common/dlmalloc.c | 24 ++++++++++++++++++++++++ include/malloc.h | 30 ++++++++++++++++++++++++++++++ test/test-main.c | 8 ++++++++ 3 files changed, 62 insertions(+) diff --git a/common/dlmalloc.c b/common/dlmalloc.c index e61f565bbab..f05d7ae83c5 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -5986,6 +5986,30 @@ void malloc_backtrace_skip(bool skip) #endif } +void malloc_backtrace_unbusy(void) +{ +#if CONFIG_IS_ENABLED(MCHECK_BACKTRACE) + in_backtrace = false; +#endif +} + +bool malloc_backtrace_is_active(bool *skipp, bool *busyp) +{ +#if CONFIG_IS_ENABLED(MCHECK_BACKTRACE) + if (skipp) + *skipp = mcheck_skip_backtrace; + if (busyp) + *busyp = in_backtrace; + return !mcheck_skip_backtrace && !in_backtrace; +#else + if (skipp) + *skipp = false; + if (busyp) + *busyp = false; + return false; +#endif +} + static const char *mcheck_caller(void) { #if CONFIG_IS_ENABLED(MCHECK_BACKTRACE) diff --git a/include/malloc.h b/include/malloc.h index ded5520ca23..05d4c06fb89 100644 --- a/include/malloc.h +++ b/include/malloc.h @@ -827,10 +827,40 @@ int malloc_log_entry(uint idx, struct mlog_entry **entryp); * * @skip: true to skip backtrace collection, false to enable it */ + +/** + * malloc_backtrace_unbusy() - Clear the backtrace reentrant guard + * + * The malloc backtrace collector sets a guard flag while collecting a + * backtrace to prevent re-entrancy. If a crash or longjmp occurs during + * collection, the guard stays set and all subsequent backtraces are + * silently skipped. Call this to reset it. + */ + +/** + * malloc_backtrace_is_active() - Check whether backtrace collection works + * + * @skipp: If non-NULL, returns true if collection is disabled via + * malloc_backtrace_skip() + * @busyp: If non-NULL, returns true if the reentrant guard is stuck + * Return: true if backtrace collection is active (neither skipped nor busy) + */ #if CONFIG_IS_ENABLED(MCHECK_HEAP_PROTECTION) void malloc_backtrace_skip(bool skip); +void malloc_backtrace_unbusy(void); +bool malloc_backtrace_is_active(bool *skipp, bool *busyp); #else static inline void malloc_backtrace_skip(bool skip) {} +static inline void malloc_backtrace_unbusy(void) {} +static inline bool malloc_backtrace_is_active(bool *skipp, bool *busyp) +{ + if (skipp) + *skipp = false; + if (busyp) + *busyp = false; + + return false; +} #endif /** diff --git a/test/test-main.c b/test/test-main.c index 09458fa91da..06bb0ff6e1b 100644 --- a/test/test-main.c +++ b/test/test-main.c @@ -464,6 +464,14 @@ static int dm_test_restore(struct device_node *of_root) */ static int test_pre_run(struct unit_test_state *uts, struct unit_test *test) { + /* + * Reset backtrace collection in case a previous test disabled it + * (e.g. stack-protector test via __stack_chk_fail) or a crash + * left the reentrant guard set. + */ + malloc_backtrace_skip(false); + malloc_backtrace_unbusy(); + ut_assertok(event_init()); /* -- 2.43.0
From: Simon Glass <sjg@chromium.org> Add 'malloc leak' subcommands for interactive heap-leak detection at the U-Boot command line: malloc leak start - snapshot current heap allocations malloc leak - count new allocations (keeps the snapshot) malloc leak end - print and count new allocations, free snapshot Each leaked allocation is printed with its address, size and caller backtrace (when mcheck is enabled). Guarded by CONFIG_CMD_MALLOC_LEAK which depends on CONFIG_SANDBOX since the snapshot uses os_malloc() for storage. Signed-off-by: Simon Glass <sjg@chromium.org> --- cmd/Kconfig | 10 +++++++ cmd/malloc.c | 65 ++++++++++++++++++++++++++++++++++++++++++ doc/develop/malloc.rst | 32 +++++++++++++++++++-- test/cmd/malloc.c | 52 +++++++++++++++++++++++++++++++++ 4 files changed, 157 insertions(+), 2 deletions(-) diff --git a/cmd/Kconfig b/cmd/Kconfig index 4af032233b9..f9d6ea46566 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -3139,6 +3139,16 @@ config CMD_MALLOC about memory allocation, such as total memory allocated and currently in use. +config CMD_MALLOC_LEAK + bool "malloc leak - Detect heap leaks" + depends on CMD_MALLOC && SANDBOX + default y + help + This adds the 'malloc leak' subcommand which snapshots the heap + and later reports any new allocations. Use 'malloc leak start' + before an operation and 'malloc leak end' afterwards to see + leaked allocations with their caller backtraces. + config CMD_MALLOC_LOG bool "malloc log - Log malloc traffic" depends on CMD_MALLOC && MCHECK_LOG diff --git a/cmd/malloc.c b/cmd/malloc.c index e7362a39bd2..6b019f4c056 100644 --- a/cmd/malloc.c +++ b/cmd/malloc.c @@ -62,6 +62,69 @@ static int __maybe_unused do_malloc_log(struct cmd_tbl *cmdtp, int flag, return 0; } +static struct malloc_leak_snap leak_snap; + +static int __maybe_unused do_malloc_leak(struct cmd_tbl *cmdtp, int flag, + int argc, char *const argv[]) +{ + if (argc < 2) { + int leaks; + + if (!leak_snap.addr) { + printf("No snapshot taken, use 'malloc leak start'\n"); + return CMD_RET_FAILURE; + } + + leaks = malloc_leak_check_count(&leak_snap); + if (leaks > 0) + printf("%d new allocs\n", leaks); + else + printf("No leaks\n"); + + return 0; + } + + if (!strcmp(argv[1], "start")) { + int ret; + + if (leak_snap.addr) + malloc_leak_check_free(&leak_snap); + ret = malloc_leak_check_start(&leak_snap); + if (ret) + return CMD_RET_FAILURE; + printf("Heap snapshot: %d allocs\n", leak_snap.count); + } else if (!strcmp(argv[1], "end")) { + int leaks; + + if (!leak_snap.addr) { + printf("No snapshot taken, use 'malloc leak start'\n"); + return CMD_RET_FAILURE; + } + + leaks = malloc_leak_check_end(&leak_snap); + if (leaks > 0) + printf("%d leaked allocs\n", leaks); + else + printf("No leaks\n"); + } else { + return CMD_RET_USAGE; + } + + return 0; +} + +#if CONFIG_IS_ENABLED(CMD_MALLOC_LEAK) +#define MALLOC_LEAK_HELP \ + "malloc leak [start|end] - detect heap leaks\n" \ + " start - snapshot current heap allocations\n" \ + " end - show allocations not in the snapshot\n" \ + " (none) - count new allocations without freeing snapshot\n" +#define MALLOC_LEAK_SUBCMD , U_BOOT_SUBCMD_MKENT(leak, 3, 1, do_malloc_leak) +#else +#define MALLOC_LEAK_HELP +#define MALLOC_LEAK_SUBCMD +#endif + #if CONFIG_IS_ENABLED(CMD_MALLOC_LOG) #define MALLOC_LOG_HELP \ "malloc log [start|stop|dump] - log malloc traffic\n" \ @@ -77,9 +140,11 @@ static int __maybe_unused do_malloc_log(struct cmd_tbl *cmdtp, int flag, U_BOOT_LONGHELP(malloc, "info - display malloc statistics\n" "malloc dump - dump heap chunks (address, size, status)\n" + MALLOC_LEAK_HELP MALLOC_LOG_HELP); U_BOOT_CMD_WITH_SUBCMDS(malloc, "malloc information", malloc_help_text, U_BOOT_SUBCMD_MKENT(info, 1, 1, do_malloc_info), U_BOOT_SUBCMD_MKENT(dump, 1, 1, do_malloc_dump) + MALLOC_LEAK_SUBCMD MALLOC_LOG_SUBCMD); diff --git a/doc/develop/malloc.rst b/doc/develop/malloc.rst index 6bb17dca7b1..ecaf8af3c3e 100644 --- a/doc/develop/malloc.rst +++ b/doc/develop/malloc.rst @@ -527,12 +527,40 @@ This makes it easy to scan an entire test suite for leaks:: $ um t --leak-check -V dm +**Interactive leak checking with the malloc command** + +The ``malloc leak`` command provides interactive leak detection at the +U-Boot command line. Take a snapshot before an operation and check +afterwards:: + + => malloc leak start + Heap snapshot: 974 allocs + => setenv foo bar + => malloc leak end + 14a2a9a0 90 sandbox_strdup:353 <-hsearch_r:403 <-env_do_env_set:130 + 14a2aa30 90 sandbox_strdup:353 <-hsearch_r:403 <-env_do_env_set:130 + 2 leaked allocs + +Use ``malloc leak`` (without arguments) to check the count without +releasing the snapshot, so you can continue testing:: + + => malloc leak start + Heap snapshot: 974 allocs + => <some operation> + => malloc leak + No leaks + => <another operation> + => malloc leak + 3 new allocs + => malloc leak end + ... + **Practical workflow** 1. Run ``um t --leak-check -V dm`` (or another suite) to find leaky tests 2. Use the caller backtrace in the ``-L`` output to find the allocation site -3. If more detail is needed, add ``malloc_dump_to_file()`` calls or enable - ``malloc_log_start()`` to trace all allocations during the operation +3. If more detail is needed, use ``malloc leak start`` / ``malloc leak end`` + interactively, or enable ``malloc_log_start()`` to trace all allocations 4. Fix the leak and verify the test passes **Dumping heap state on exit** diff --git a/test/cmd/malloc.c b/test/cmd/malloc.c index 75e8afdec63..95809845dd7 100644 --- a/test/cmd/malloc.c +++ b/test/cmd/malloc.c @@ -54,6 +54,58 @@ static int cmd_test_malloc_dump(struct unit_test_state *uts) } CMD_TEST(cmd_test_malloc_dump, UTF_CONSOLE); +/* Test 'malloc leak' command using the C API directly */ +static int cmd_test_malloc_leak(struct unit_test_state *uts) +{ + struct malloc_leak_snap snap = {}; + ulong chunk_addr; + size_t chunk_sz; + void *ptr; + int ret; + + /* Take a snapshot, then check with no leaks */ + ut_assertok(malloc_leak_check_start(&snap)); + ut_assert(snap.count > 0); + ut_asserteq(0, malloc_leak_check_count(&snap)); + + /* Allocate something and check it is detected */ + ptr = malloc(0x42); + ut_assertnonnull(ptr); + ut_asserteq(1, malloc_leak_check_count(&snap)); + + /* Verify freeing clears the leak */ + free(ptr); + ut_asserteq(0, malloc_leak_check_count(&snap)); + + /* Re-allocate so end has something to print */ + ptr = malloc(0x42); + ut_assertnonnull(ptr); + + /* End should print the leaked allocation and free the snapshot */ + ret = malloc_leak_check_end(&snap); + ut_asserteq(1, ret); + + /* + * Check the output line shows the correct chunk address and chunk + * size. The chunk address is the malloc pointer minus the mcheck + * header. The caller name is only available with mcheck. + */ + chunk_addr = (ulong)ptr - malloc_mcheck_hdr_size(); + chunk_sz = malloc_chunk_size(ptr); + if (IS_ENABLED(CONFIG_MCHECK_HEAP_PROTECTION)) + ut_assert_nextlinen(" %lx %zx %s:", chunk_addr, chunk_sz, + __func__); + else + ut_assert_nextlinen(" %lx %zx ", chunk_addr, chunk_sz); + ut_assert_console_end(); + ut_assertnull(snap.addr); + + free(ptr); + + return 0; +} +CMD_TEST(cmd_test_malloc_leak, UTF_CONSOLE); + #if CONFIG_IS_ENABLED(MCHECK_LOG) /* Test 'malloc log' command */ static int cmd_test_malloc_log(struct unit_test_state *uts) -- 2.43.0
From: Simon Glass <sjg@chromium.org> Add a --leak-check flag to the pytest command line that passes -L to every ut invocation, enabling per-test heap-leak detection. Leaked allocations are printed with their address, size and caller backtrace. For example: test/py/test.py -B sandbox --leak-check -k dm_test_acpi_bgrt Signed-off-by: Simon Glass <sjg@chromium.org> --- test/py/conftest.py | 3 +++ test/py/tests/test_ut.py | 6 ++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/test/py/conftest.py b/test/py/conftest.py index 47a0d112e51..303a697d76a 100644 --- a/test/py/conftest.py +++ b/test/py/conftest.py @@ -106,6 +106,8 @@ def pytest_addoption(parser): help='Disable console timeout (useful for debugging)') parser.addoption('--no-full', default=False, action='store_true', help='Skip flat-tree tests (run live-tree only)') + parser.addoption('--leak-check', default=False, action='store_true', + help='Check for memory leaks around each unit test') parser.addoption('--malloc-dump', default=None, help='Write malloc dump to file on exit') @@ -364,6 +366,7 @@ def pytest_configure(config): ubconfig.allow_exceptions = config.getoption('allow_exceptions') ubconfig.no_timeout = config.getoption('no_timeout') ubconfig.no_full = config.getoption('no_full') + ubconfig.leak_check = config.getoption('leak_check') ubconfig.malloc_dump = config.getoption('malloc_dump') env_vars = ( diff --git a/test/py/tests/test_ut.py b/test/py/tests/test_ut.py index e64ccd407a0..21a948b4ac6 100644 --- a/test/py/tests/test_ut.py +++ b/test/py/tests/test_ut.py @@ -142,11 +142,13 @@ def test_ut(ubman, ut_subtest): execute command 'ut foo bar' """ + flags = '-L ' if ubman.config.leak_check else '' + if ut_subtest == 'hush hush_test_simple_dollar': # ut hush hush_test_simple_dollar prints "Unknown command" on purpose. with ubman.disable_check('unknown_command'): - output = ubman.run_command('ut ' + ut_subtest) + output = ubman.run_command(f'ut {flags}' + ut_subtest) assert 'Unknown command \'quux\' - try \'help\'' in output else: - output = ubman.run_command('ut ' + ut_subtest) + output = ubman.run_command(f'ut {flags}' + ut_subtest) assert output.endswith('failures: 0') -- 2.43.0
From: Simon Glass <sjg@chromium.org> The output buffer allocated in scmi_base_discover_list_protocols_int() is freed on the error path but not on the success path. Add the missing free(out) before returning on success. Fixes: ec8727b7e1c1 ("firmware: scmi: implement SCMI base protocol") Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/firmware/scmi/base.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/firmware/scmi/base.c b/drivers/firmware/scmi/base.c index 78ee2ffd2da..d591b23d158 100644 --- a/drivers/firmware/scmi/base.c +++ b/drivers/firmware/scmi/base.c @@ -304,6 +304,7 @@ static int scmi_base_discover_list_protocols_int(struct udevice *dev, } while (cur < num_protocols); *protocols = buf; + free(out); return num_protocols; err: -- 2.43.0
From: Simon Glass <sjg@chromium.org> The sandbox I2C PMIC emulator allocates a register buffer in its probe function but has no remove function to free it. Add a remove handler to free the buffer when the device is removed. Fixes: eb7387ae14ef ("sandbox: pmic: Correct i2c pmic emulator platdata method") Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/power/pmic/i2c_pmic_emul.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/drivers/power/pmic/i2c_pmic_emul.c b/drivers/power/pmic/i2c_pmic_emul.c index 6e81b9c3427..dd0ec39ef48 100644 --- a/drivers/power/pmic/i2c_pmic_emul.c +++ b/drivers/power/pmic/i2c_pmic_emul.c @@ -146,6 +146,16 @@ static int sandbox_i2c_pmic_probe(struct udevice *emul) return 0; } +static int sandbox_i2c_pmic_remove(struct udevice *emul) +{ + struct sandbox_i2c_pmic_plat_data *plat = dev_get_plat(emul); + + free(plat->reg); + plat->reg = NULL; + + return 0; +} + struct dm_i2c_ops sandbox_i2c_pmic_emul_ops = { .xfer = sandbox_i2c_pmic_xfer, }; @@ -161,6 +171,7 @@ U_BOOT_DRIVER(sandbox_i2c_pmic_emul) = { .of_match = sandbox_i2c_pmic_ids, .of_to_plat = sandbox_i2c_pmic_of_to_plat, .probe = sandbox_i2c_pmic_probe, + .remove = sandbox_i2c_pmic_remove, .plat_auto = sizeof(struct sandbox_i2c_pmic_plat_data), .ops = &sandbox_i2c_pmic_emul_ops, }; -- 2.43.0
From: Simon Glass <sjg@chromium.org> sandbox_sf_bind_emul() allocates a device name with strdup() but does not mark it with DM_FLAG_NAME_ALLOCED, so device_unbind() never frees it. Add the missing device_set_name_alloced() call. Fixes: b6c2956defb4 ("dm: sf: sandbox: Convert SPI flash driver to driver model") Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/mtd/spi/sandbox.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/mtd/spi/sandbox.c b/drivers/mtd/spi/sandbox.c index 0b56312bb85..573ce463294 100644 --- a/drivers/mtd/spi/sandbox.c +++ b/drivers/mtd/spi/sandbox.c @@ -553,6 +553,7 @@ int sandbox_sf_bind_emul(struct sandbox_state *state, int busnum, int cs, spec, ret); return ret; } + device_set_name_alloced(emul); state->spi[busnum][cs].emul = emul; return 0; -- 2.43.0
From: Simon Glass <sjg@chromium.org> decode_regions() allocates hose->regions in pci_uclass_pre_probe() but the array is never freed when the PCI bus device is removed. Add a pre_remove() handler to free it. Fixes: e002474158d1 ("pci: pci-uclass: Dynamically allocate the PCI regions") Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/pci/pci-uclass.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index c370f8c6400..27bc92f0e4c 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -1874,6 +1874,16 @@ int pci_sriov_get_totalvfs(struct udevice *pdev) } #endif /* SRIOV */ +static int pci_uclass_pre_remove(struct udevice *bus) +{ + struct pci_controller *hose = dev_get_uclass_priv(bus); + + free(hose->regions); + hose->regions = NULL; + + return 0; +} + UCLASS_DRIVER(pci) = { .id = UCLASS_PCI, .name = "pci", @@ -1881,6 +1891,7 @@ UCLASS_DRIVER(pci) = { .post_bind = dm_scan_fdt_dev, .pre_probe = pci_uclass_pre_probe, .post_probe = pci_uclass_post_probe, + .pre_remove = pci_uclass_pre_remove, .child_post_bind = pci_uclass_child_post_bind, .per_device_auto = sizeof(struct pci_controller), .per_child_plat_auto = sizeof(struct pci_child_plat), -- 2.43.0
From: Simon Glass <sjg@chromium.org> Fix three memory leaks in the ACPI device property tests: - free_context() does not free ctx->base, unlike the version in acpigen.c - dm_test_acpi_dp_gpio() does not call free_context() - dm_test_acpi_dp_copy() does not call free_context() Fixes: 0e5a0a00d6e4 ("acpi: Support writing Device Properties objects via _DSD") Signed-off-by: Simon Glass <sjg@chromium.org> --- test/dm/acpi_dp.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/dm/acpi_dp.c b/test/dm/acpi_dp.c index 038806004b5..90921329556 100644 --- a/test/dm/acpi_dp.c +++ b/test/dm/acpi_dp.c @@ -36,6 +36,7 @@ static int alloc_context(struct acpi_ctx **ctxp) static void free_context(struct acpi_ctx **ctxp) { + free((*ctxp)->base); free(*ctxp); *ctxp = NULL; } @@ -419,6 +420,8 @@ static int dm_test_acpi_dp_gpio(struct unit_test_state *uts) ut_asserteq(ZERO_OP, pptr[0x1b]); ut_asserteq(ONE_OP, pptr[0x1c]); + free_context(&ctx); + return 0; } DM_TEST(dm_test_acpi_dp_gpio, 0); @@ -486,6 +489,8 @@ static int dm_test_acpi_dp_copy(struct unit_test_state *uts) ut_asserteq(STRING_PREFIX, ptr[0x8e]); ut_asserteq_str("sunrise ohoka", (char *)(ptr + 0x8f)); + free_context(&ctx); + return 0; } DM_TEST(dm_test_acpi_dp_copy, UTF_SCAN_PDATA | UTF_SCAN_FDT); -- 2.43.0
From: Simon Glass <sjg@chromium.org> acpi_reset_items() resets the item count without freeing the buffers allocated by add_item(). Free them before resetting. Also add missing free(buf) calls in dm_test_acpi_fill_ssdt(), dm_test_acpi_fill_madt() and dm_test_acpi_inject_dsdt() which allocate a buffer with malloc() but never free it. Fixes: 18434aec1b69 ("acpi: Don't reset the tables with every new generation") Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/core/acpi.c | 6 ++++++ test/dm/acpi.c | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/drivers/core/acpi.c b/drivers/core/acpi.c index 4763963914b..efb935ed6a1 100644 --- a/drivers/core/acpi.c +++ b/drivers/core/acpi.c @@ -377,6 +377,12 @@ int acpi_inject_dsdt(struct acpi_ctx *ctx) void acpi_reset_items(void) { + int i; + + for (i = 0; i < item_count; i++) { + free(acpi_item[i].buf); + acpi_item[i].buf = NULL; + } item_count = 0; } diff --git a/test/dm/acpi.c b/test/dm/acpi.c index 588a518bc4f..9eca5df967b 100644 --- a/test/dm/acpi.c +++ b/test/dm/acpi.c @@ -596,6 +596,8 @@ static int dm_test_acpi_fill_ssdt(struct unit_test_state *uts) ut_asserteq('z', buf[4]); + free(buf); + return 0; } DM_TEST(dm_test_acpi_fill_ssdt, UTF_SCAN_PDATA | UTF_SCAN_FDT); @@ -622,6 +624,8 @@ static int dm_test_acpi_fill_madt(struct unit_test_state *uts) ut_asserteq('z', buf[1]); + free(buf); + return 0; } @@ -654,6 +658,8 @@ static int dm_test_acpi_inject_dsdt(struct unit_test_state *uts) ut_asserteq('z', buf[4]); + free(buf); + return 0; } DM_TEST(dm_test_acpi_inject_dsdt, UTF_SCAN_PDATA | UTF_SCAN_FDT); -- 2.43.0
From: Simon Glass <sjg@chromium.org> show_splash() calls map_to_sysmem() to convert the splash image pointer to a physical address but never unmaps it, leaking a sandbox memory-mapping entry. Add the missing unmap_sysmem() call. Fixes: 84e63abfff67 ("video: Support showing the U-Boot logo") Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/video/video-uclass.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c index b492ff313aa..20d98798520 100644 --- a/drivers/video/video-uclass.c +++ b/drivers/video/video-uclass.c @@ -661,6 +661,7 @@ static int show_splash(struct udevice *dev) data = video_image_getptr(u_boot); ret = video_bmp_display(dev, map_to_sysmem(data), -4, 4, true); + unmap_sysmem(data); return 0; } -- 2.43.0
participants (1)
-
Simon Glass