[PATCH 00/33] Fix memory leaks and test pollution in sandbox tests
From: Simon Glass <sjg@chromium.org> Running the full sandbox test suite with leak checking (-M) exposes several classes of bugs: the mcheck allocation registry overflows after ~40 tests, driver-model state leaks ~640 chunks per 'ut' command, stale pointers are left behind by tests that do not clean up, and heap fragmentation breaks tests that assume a clean arena. This series addresses each layer: - mcheck tooling: bump the registry, expose overflow/count/largest-free APIs, and report them from 'malloc info' - DM state save/restore: implement the documented (but missing) save/restore of the global driver-model state around UTF_DM tests, eliminating the per-command leak - Driver leak fixes: free resources in remove paths for virtio, USB, blk, bootdev, bootctl, and expo/cedit - Test pollution fixes: clear stale bootflows, reset video sync/font state, dispose live trees, and clean up DM state in tests that leave it dirty There are still quite a few leaks remaining, but this series makes a significant improvement. Simon Glass (33): malloc: Double the allocation registry size malloc: Expose registry-overflow state to tests malloc: Report mcheck-registry entry count malloc: Expose the largest-contiguous free region malloc: Find aligned allocations using the registry virtio: Free mmio base when removing sandbox virtio emul virtio: Delete vqs when removing a device virtio: emul_blk: Free disk data on remove usb: Drain finished scan threads after run_threads blk: Mark partition name as alloced test: common: Target largest free chunk in heap-pressure tests test: Save and restore global DM state around UTF_DM tests test: video: Restore auto-sync at end of video_sync_damage test: video: Reselect default font at start of video_silence test: usb: Reset usb_started at start of usb_stop test boot: Close the filesystem after reading a bootflow boot: Free the logo buffer when freeing a bootflow bootctl: Destroy the UI expo when the device is removed bootctl: simple_state: Free items alist on remove boot: Replace the current cedit expo on reload boot: cedit: Leave manual-sync off with exit without save upl: Dispose of live trees after use test: dm: Dispose tree in livetree_ensure() test: dm: Dispose tree in oftree_new test: dm: Free trees in ofnode_too_many() test: dm: Release resources in oftree_to_fdt(() test test: upl: Dispose oftrees in upl_test_base() test: lib: Dispose tree in json_to_fdt_luks2() test: boot: Clean up DM state after bootflow_cmdline() test: boot: Clean up DM state after bootstd_images() test: boot: Clean up DM state after bootstd_adhoc() test: setexpr: Unset test env var in str_long() test: Clear bootstd cur_bootflow after each test boot/bootctl/bootctl-uclass.c | 14 ++++++++ boot/bootctl/simple_state.c | 11 +++++++ boot/bootdev-uclass.c | 9 ++++++ boot/bootflow.c | 1 + boot/cedit.c | 2 ++ cmd/cedit.c | 2 ++ cmd/malloc.c | 3 ++ cmd/upl.c | 2 ++ common/dlmalloc.c | 59 ++++++++++++++++++++++++++-------- common/mcheck_core.inc.h | 8 ++--- drivers/block/blk-uclass.c | 1 + drivers/usb/host/usb-uclass.c | 6 ++++ drivers/virtio/emul_blk.c | 10 ++++++ drivers/virtio/sandbox_emul.c | 3 ++ drivers/virtio/virtio-uclass.c | 7 ++++ drivers/virtio/virtio_blk.c | 2 +- drivers/virtio/virtio_net.c | 2 +- drivers/virtio/virtio_rng.c | 2 +- drivers/virtio/virtio_scsi.c | 2 +- include/malloc.h | 31 ++++++++++++++++++ include/test/test.h | 10 ++++++ include/virtio.h | 8 +++++ test/boot/bootflow.c | 6 ++-- test/boot/upl.c | 2 ++ test/cmd/malloc.c | 9 ++++++ test/cmd/setexpr.c | 1 + test/common/malloc.c | 33 ++++++++++++------- test/dm/ofnode.c | 9 ++++++ test/dm/usb.c | 8 +++++ test/dm/video.c | 9 ++++++ test/lib/json.c | 1 + test/test-main.c | 56 ++++++++++++++++++++++++++------ 32 files changed, 285 insertions(+), 44 deletions(-) --- base-commit: a71b77d407ece89d306afe21de1e0b42dc83f5d2 branch: memc -- 2.43.0
From: Simon Glass <sjg@chromium.org> When running the full test suite, the live allocation count can exceed 20000, overflowing the mcheck registry. Once that happens, further allocations are still returned but their headers are not registered, so leak reports cannot recover the caller string and simply show a blank caller field. The failure is easy to miss because the "registry overflow" message is printed only once and may be silenced under the unit-test console. Bump REGISTRY_SZ to 40000 so the whole suite fits. The array lives in .data and only costs an extra 160KiB for sandbox builds, which is acceptable for a debug-only feature. Signed-off-by: Simon Glass <sjg@chromium.org> --- common/mcheck_core.inc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/mcheck_core.inc.h b/common/mcheck_core.inc.h index 65f83836d61..968b058ba6d 100644 --- a/common/mcheck_core.inc.h +++ b/common/mcheck_core.inc.h @@ -71,7 +71,7 @@ #define PADDINGFLOOD ((char)0x58) // Full test suite can exceed 10000 concurrent allocations -#define REGISTRY_SZ 20000 +#define REGISTRY_SZ 40000 #define CANARY_DEPTH 2 // avoid problems with BSS at early stage: -- 2.43.0
From: Simon Glass <sjg@chromium.org> If the mcheck registry ever overflows, later allocations are not recorded so find_mcheck_hdr_in_chunk() cannot find their headers. Leak reports then print an empty caller column, and cmd_test_malloc_leak() fails with a confusing assertion where the expected output shows a function name and the actual output shows trailing whitespace. The 'registry overflow' printf() that marks this state is printed once and goes to the silenced test console, which makes the condition hard to notice. Promote the one-shot overflow flag to file scope expose it via a new malloc_mcheck_overflow() function. Check it at the top of cmd_test_malloc_leak() so any earlier test that exhausts the registry now produces a direct failure in the leak test rather than a misleading one based on missing caller strings. Signed-off-by: Simon Glass <sjg@chromium.org> --- common/dlmalloc.c | 5 +++++ common/mcheck_core.inc.h | 6 +++--- include/malloc.h | 16 ++++++++++++++++ test/cmd/malloc.c | 6 ++++++ 4 files changed, 30 insertions(+), 3 deletions(-) diff --git a/common/dlmalloc.c b/common/dlmalloc.c index f05d7ae83c5..ae1e6d27d08 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -6010,6 +6010,11 @@ bool malloc_backtrace_is_active(bool *skipp, bool *busyp) #endif } +bool malloc_mcheck_overflow(void) +{ + return mcheck_registry_full; +} + static const char *mcheck_caller(void) { #if CONFIG_IS_ENABLED(MCHECK_BACKTRACE) diff --git a/common/mcheck_core.inc.h b/common/mcheck_core.inc.h index 968b058ba6d..9f13945cde8 100644 --- a/common/mcheck_core.inc.h +++ b/common/mcheck_core.inc.h @@ -79,6 +79,7 @@ static char mcheck_pedantic_flag __section(".data") = 0; static void *mcheck_registry[REGISTRY_SZ] __section(".data") = {0}; static size_t mcheck_chunk_count __section(".data") = 0; static size_t mcheck_chunk_count_max __section(".data") = 0; +static bool mcheck_registry_full __section(".data"); typedef unsigned long long mcheck_elem; typedef struct { @@ -200,7 +201,6 @@ static void *mcheck_allocated_helper(void *altoghether_ptr, size_t customer_sz, size_t alignment, int clean_content, const char *caller) { - static bool overflow_msg_shown; const size_t slop = alignment ? mcheck_evaluate_memalign_prefix_size(alignment) - sizeof(struct mcheck_hdr) : 0; struct mcheck_hdr *hdr = (struct mcheck_hdr *)((char *)altoghether_ptr + slop); @@ -239,8 +239,8 @@ static void *mcheck_allocated_helper(void *altoghether_ptr, size_t customer_sz, return payload; // normal end } - if (!overflow_msg_shown) { - overflow_msg_shown = true; + if (!mcheck_registry_full) { + mcheck_registry_full = true; printf("\n\nERROR: mcheck registry overflow, pedantic check would be incomplete!\n\n"); } return payload; diff --git a/include/malloc.h b/include/malloc.h index 05d4c06fb89..0a780fc03c3 100644 --- a/include/malloc.h +++ b/include/malloc.h @@ -863,6 +863,22 @@ static inline bool malloc_backtrace_is_active(bool *skipp, bool *busyp) } #endif +/** + * malloc_mcheck_overflow() - Check whether the mcheck registry filled + * + * The mcheck code registers each allocation's header in a fixed-size array so + * that leak reporting and pedantic checking can find it. If the array is ever + * exhausted, later allocations are still returned but their caller info cannot + * be recovered. + * + * Return: true if the registry has overflowed at any point + */ +#if CONFIG_IS_ENABLED(MCHECK_HEAP_PROTECTION) +bool malloc_mcheck_overflow(void); +#else +static inline bool malloc_mcheck_overflow(void) { return false; } +#endif + /** * malloc_chunk_size() - Return the dlmalloc chunk size for an allocation * diff --git a/test/cmd/malloc.c b/test/cmd/malloc.c index 95809845dd7..db4350afe68 100644 --- a/test/cmd/malloc.c +++ b/test/cmd/malloc.c @@ -63,6 +63,12 @@ static int cmd_test_malloc_leak(struct unit_test_state *uts) void *ptr; int ret; + /* + * If the mcheck registry ever overflowed, later allocations are not + * registered and leak reports cannot recover the caller string. + */ + ut_assert(!malloc_mcheck_overflow()); + /* Take a snapshot, then check with no leaks */ ut_assertok(malloc_leak_check_start(&snap)); ut_assert(snap.count > 0); -- 2.43.0
From: Simon Glass <sjg@chromium.org> When tracking down heap-registry overflows it is useful to see how many entries are currently tracked, so the limit can be compared at a glance. Expose malloc_mcheck_count() as a public accessor over the existing mcheck_chunk_count variable, and print its value with the 'malloc info' command when CONFIG_MCHECK_HEAP_PROTECTION is enabled. Extend cmd_test_malloc_info() to check the extra line when the feature is on. Signed-off-by: Simon Glass <sjg@chromium.org> --- cmd/malloc.c | 2 ++ common/dlmalloc.c | 5 +++++ include/malloc.h | 2 ++ test/cmd/malloc.c | 2 ++ 4 files changed, 11 insertions(+) diff --git a/cmd/malloc.c b/cmd/malloc.c index 6b019f4c056..7de421c0ae8 100644 --- a/cmd/malloc.c +++ b/cmd/malloc.c @@ -27,6 +27,8 @@ static int do_malloc_info(struct cmd_tbl *cmdtp, int flag, int argc, printf("malloc count = %lu\n", info.malloc_count); printf("free count = %lu\n", info.free_count); printf("realloc count = %lu\n", info.realloc_count); + if (IS_ENABLED(CONFIG_MCHECK_HEAP_PROTECTION)) + printf("mcheck count = %zu\n", malloc_mcheck_count()); return 0; } diff --git a/common/dlmalloc.c b/common/dlmalloc.c index ae1e6d27d08..895aa215228 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -6015,6 +6015,11 @@ bool malloc_mcheck_overflow(void) return mcheck_registry_full; } +size_t malloc_mcheck_count(void) +{ + return mcheck_chunk_count; +} + static const char *mcheck_caller(void) { #if CONFIG_IS_ENABLED(MCHECK_BACKTRACE) diff --git a/include/malloc.h b/include/malloc.h index 0a780fc03c3..787ec999f5e 100644 --- a/include/malloc.h +++ b/include/malloc.h @@ -875,8 +875,10 @@ static inline bool malloc_backtrace_is_active(bool *skipp, bool *busyp) */ #if CONFIG_IS_ENABLED(MCHECK_HEAP_PROTECTION) bool malloc_mcheck_overflow(void); +size_t malloc_mcheck_count(void); #else static inline bool malloc_mcheck_overflow(void) { return false; } +static inline size_t malloc_mcheck_count(void) { return 0; } #endif /** diff --git a/test/cmd/malloc.c b/test/cmd/malloc.c index db4350afe68..812f7530b79 100644 --- a/test/cmd/malloc.c +++ b/test/cmd/malloc.c @@ -28,6 +28,8 @@ static int cmd_test_malloc_info(struct unit_test_state *uts) ut_assert_nextlinen("malloc count = "); ut_assert_nextlinen("free count = "); ut_assert_nextlinen("realloc count = "); + if (IS_ENABLED(CONFIG_MCHECK_HEAP_PROTECTION)) + ut_assert_nextlinen("mcheck count = "); ut_assert_console_end(); return 0; -- 2.43.0
From: Simon Glass <sjg@chromium.org> Heap-pressure tests like common_test_malloc_very_large() assume almost the whole heap is available as one contiguous free chunk. Even a single, stray, leaked allocation in the middle of the area breaks that assumption and fails the test, which makes the tests flakily depend on every earlier test running with a perfect cleanup. Add malloc_largest_free() which walks all segments (and the top and dv chunks) and returns the biggest free chunk size, minus the per-chunk dlmalloc overhead. Report it from 'malloc info' so the value is easy to see while diagnosing fragmentation, and check for the new line in cmd_test_malloc_info() Signed-off-by: Simon Glass <sjg@chromium.org> --- cmd/malloc.c | 1 + common/dlmalloc.c | 29 +++++++++++++++++++++++++++++ include/malloc.h | 13 +++++++++++++ test/cmd/malloc.c | 1 + 4 files changed, 44 insertions(+) diff --git a/cmd/malloc.c b/cmd/malloc.c index 7de421c0ae8..94d0f9427d6 100644 --- a/cmd/malloc.c +++ b/cmd/malloc.c @@ -24,6 +24,7 @@ static int do_malloc_info(struct cmd_tbl *cmdtp, int flag, int argc, printf("total bytes = %s\n", format_size(buf, info.total_bytes)); printf("in use bytes = %s\n", format_size(buf, info.in_use_bytes)); + printf("largest free = %s\n", format_size(buf, malloc_largest_free())); printf("malloc count = %lu\n", info.malloc_count); printf("free count = %lu\n", info.free_count); printf("realloc count = %lu\n", info.realloc_count); diff --git a/common/dlmalloc.c b/common/dlmalloc.c index 895aa215228..6d0bdc5b64e 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -7900,6 +7900,35 @@ void malloc_leak_check_free(struct malloc_leak_snap *snap) snap->count = 0; } +size_t malloc_largest_free(void) +{ + size_t largest = 0; + mstate m = gm; + msegmentptr s; + + if (m->topsize > largest) + largest = m->topsize; + if (m->dvsize > largest) + largest = m->dvsize; + + for (s = &m->seg; s != 0; s = s->next) { + mchunkptr q = align_as_chunk(s->base); + + while (segment_holds(s, q) && q != m->top && + q->head != FENCEPOST_HEAD) { + if (!is_inuse(q)) { + size_t sz = chunksize(q); + + if (sz > largest) + largest = sz; + } + q = next_chunk(q); + } + } + + return largest > CHUNK_OVERHEAD ? largest - CHUNK_OVERHEAD : 0; +} + int initf_malloc(void) { #if CONFIG_IS_ENABLED(SYS_MALLOC_F) diff --git a/include/malloc.h b/include/malloc.h index 787ec999f5e..599440246b4 100644 --- a/include/malloc.h +++ b/include/malloc.h @@ -892,6 +892,19 @@ static inline size_t malloc_mcheck_count(void) { return 0; } */ size_t malloc_chunk_size(void *ptr); +/** + * malloc_largest_free() - Return the size of the largest free chunk + * + * Walks the heap and returns the largest contiguous free region that + * malloc() could currently hand out, minus the per-chunk dlmalloc + * overhead. Any mcheck header/canary overhead still comes off the top + * of the caller's request. Useful for tests that want to allocate "as + * much as possible" without tripping over fragmentation. + * + * Return: approximate largest request bytes malloc() would satisfy + */ +size_t malloc_largest_free(void); + /** * malloc_mcheck_hdr_size() - Return the size of the mcheck header * diff --git a/test/cmd/malloc.c b/test/cmd/malloc.c index 812f7530b79..e84033b3ce9 100644 --- a/test/cmd/malloc.c +++ b/test/cmd/malloc.c @@ -25,6 +25,7 @@ static int cmd_test_malloc_info(struct unit_test_state *uts) ut_assertok(run_command("malloc info", 0)); ut_assert_nextlinen("total bytes = "); ut_assert_nextlinen("in use bytes = "); + ut_assert_nextlinen("largest free = "); ut_assert_nextlinen("malloc count = "); ut_assert_nextlinen("free count = "); ut_assert_nextlinen("realloc count = "); -- 2.43.0
From: Simon Glass <sjg@chromium.org> The leak-check printer reads the mcheck header from the start of each chunk, but for memalign() allocations the header is offset by aln_skip to satisfy alignment. Those chunks end up with no caller in the leak report, which hides up to several dozen leaks per test under a 'no-trace' bucket. Use find_mcheck_hdr_in_chunk(), which walks the registry and accounts for aln_skip, the same way malloc_dump_impl() already does. Signed-off-by: Simon Glass <sjg@chromium.org> --- common/dlmalloc.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/common/dlmalloc.c b/common/dlmalloc.c index 6d0bdc5b64e..6cc703f06fc 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -7833,21 +7833,15 @@ static void print_new_allocs(struct malloc_leak_snap *snap) #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). + * For memalign()ed chunks the header is + * offset by aln_skip, so use the registry- + * based lookup rather than assuming the + * header is at chunk2mem(q). */ - struct mcheck_hdr *hdr = mem; - int j; + struct mcheck_hdr *hdr; - for (j = 0; j < CANARY_DEPTH; j++) - if (hdr->canary.elems[j] != MAGICWORD) - break; - if (j == CANARY_DEPTH && hdr->caller[0]) + hdr = find_mcheck_hdr_in_chunk(mem, sz); + if (hdr && hdr->caller[0]) caller = hdr->caller; #endif printf(" %lx %zx %s\n", (ulong)mem, sz, -- 2.43.0
From: Simon Glass <sjg@chromium.org> sandbox_emul_of_to_plat() used memalign() for its emulated MMIO region, but sandbox_emul_remove() only unregisters the MMIO callback; the buffer itself (several KB per device) is not freed. Free it on removal. Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/virtio/sandbox_emul.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/virtio/sandbox_emul.c b/drivers/virtio/sandbox_emul.c index 2c75fd546d1..cde666ce99d 100644 --- a/drivers/virtio/sandbox_emul.c +++ b/drivers/virtio/sandbox_emul.c @@ -283,7 +283,10 @@ static int sandbox_emul_of_to_plat(struct udevice *dev) static int sandbox_emul_remove(struct udevice *dev) { + struct sandbox_emul_priv *priv = dev_get_priv(dev); + sandbox_mmio_remove(dev); + free(priv->mmio.base); return 0; } -- 2.43.0
From: Simon Glass <sjg@chromium.org> The rng, net, blk and scsi drivers all use virtio_reset() as their remove() method. virtio_reset() only uses the transport's reset() operation, leaving the virtual queues still allocated (12 KB per virtqueue). Every probe/remove cycle of a virtio device therefore fails to free its queues. Add a remove() method to delete the queues before resetting. Update the four drivers to use it. Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/virtio/virtio-uclass.c | 7 +++++++ drivers/virtio/virtio_blk.c | 2 +- drivers/virtio/virtio_net.c | 2 +- drivers/virtio/virtio_rng.c | 2 +- drivers/virtio/virtio_scsi.c | 2 +- include/virtio.h | 8 ++++++++ 6 files changed, 19 insertions(+), 4 deletions(-) diff --git a/drivers/virtio/virtio-uclass.c b/drivers/virtio/virtio-uclass.c index 86a5d8af66b..207002bd910 100644 --- a/drivers/virtio/virtio-uclass.c +++ b/drivers/virtio/virtio-uclass.c @@ -105,6 +105,13 @@ int virtio_reset(struct udevice *vdev) return ops->reset(vdev->parent); } +int virtio_remove(struct udevice *vdev) +{ + virtio_del_vqs(vdev); + + return virtio_reset(vdev); +} + int virtio_get_features(struct udevice *vdev, u64 *features) { struct dm_virtio_ops *ops; diff --git a/drivers/virtio/virtio_blk.c b/drivers/virtio/virtio_blk.c index 3dd0cf36268..236f807480e 100644 --- a/drivers/virtio/virtio_blk.c +++ b/drivers/virtio/virtio_blk.c @@ -222,7 +222,7 @@ U_BOOT_DRIVER(virtio_blk) = { .ops = &virtio_blk_ops, .bind = virtio_blk_bind, .probe = virtio_blk_probe, - .remove = virtio_reset, + .remove = virtio_remove, .priv_auto = sizeof(struct virtio_blk_priv), .flags = DM_FLAG_ACTIVE_DMA, }; diff --git a/drivers/virtio/virtio_net.c b/drivers/virtio/virtio_net.c index 7a94c8a528f..d49dab3a6ed 100644 --- a/drivers/virtio/virtio_net.c +++ b/drivers/virtio/virtio_net.c @@ -233,7 +233,7 @@ U_BOOT_DRIVER(virtio_net) = { .id = UCLASS_ETH, .bind = virtio_net_bind, .probe = virtio_net_probe, - .remove = virtio_reset, + .remove = virtio_remove, .ops = &virtio_net_ops, .priv_auto = sizeof(struct virtio_net_priv), .plat_auto = sizeof(struct eth_pdata), diff --git a/drivers/virtio/virtio_rng.c b/drivers/virtio/virtio_rng.c index 90a371a59cc..35e88266cb6 100644 --- a/drivers/virtio/virtio_rng.c +++ b/drivers/virtio/virtio_rng.c @@ -89,7 +89,7 @@ U_BOOT_DRIVER(virtio_rng) = { .id = UCLASS_RNG, .bind = virtio_rng_bind, .probe = virtio_rng_probe, - .remove = virtio_reset, + .remove = virtio_remove, .ops = &virtio_rng_ops, .priv_auto = sizeof(struct virtio_rng_priv), .flags = DM_FLAG_ACTIVE_DMA, diff --git a/drivers/virtio/virtio_scsi.c b/drivers/virtio/virtio_scsi.c index ee54f1c7449..185081640a7 100644 --- a/drivers/virtio/virtio_scsi.c +++ b/drivers/virtio/virtio_scsi.c @@ -243,7 +243,7 @@ U_BOOT_DRIVER(virtio_scsi) = { .priv_auto = sizeof(struct virtio_scsi_priv), .ops = &virtio_scsi_ops, .probe = virtio_scsi_probe, - .remove = virtio_reset, + .remove = virtio_remove, .bind = virtio_scsi_bind, .flags = DM_FLAG_ACTIVE_DMA, }; diff --git a/include/virtio.h b/include/virtio.h index ecbd16a98a7..b58ef880b4f 100644 --- a/include/virtio.h +++ b/include/virtio.h @@ -294,6 +294,14 @@ int virtio_set_status(struct udevice *vdev, u8 status); */ int virtio_reset(struct udevice *vdev); +/** + * virtio_remove() - remove a virtio device by deleting its vqs and resetting it + * + * @vdev: the real virtio device + * Return: 0 if OK, -ve on error + */ +int virtio_remove(struct udevice *vdev); + /** * virtio_get_features() - get the array of feature bits for this device * -- 2.43.0
From: Simon Glass <sjg@chromium.org> The probe function allocates 1MB for the emulated disk contents but there is no corresponding free on remove, so the memory leaks every time the device is torn down. This shows up as a 1MB leak per test in several bootstd tests. Add a remove method to free the buffer. Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/virtio/emul_blk.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/virtio/emul_blk.c b/drivers/virtio/emul_blk.c index 1d42b34b8ab..e9eab4a6470 100644 --- a/drivers/virtio/emul_blk.c +++ b/drivers/virtio/emul_blk.c @@ -131,6 +131,15 @@ static int virtio_blk_emul_probe(struct udevice *dev) return 0; } +static int virtio_blk_emul_remove(struct udevice *dev) +{ + struct virtio_blk_emul_priv *priv = dev_get_priv(dev); + + free(priv->disk_data); + + return 0; +} + static struct virtio_emul_ops blk_emul_ops = { .process_request = blk_emul_process_request, .get_config = blk_emul_get_config, @@ -148,6 +157,7 @@ U_BOOT_DRIVER(virtio_blk_emul) = { .id = UCLASS_VIRTIO_EMUL, .of_match = virtio_blk_emul_ids, .probe = virtio_blk_emul_probe, + .remove = virtio_blk_emul_remove, .ops = &blk_emul_ops, .priv_auto = sizeof(struct virtio_blk_emul_priv), }; -- 2.43.0
From: Simon Glass <sjg@chromium.org> run_threads() loops until uthread_grp_done() reports that every thread in the group has finished. But uthread_schedule() only frees 'done' threads it walks past on the way to the next runnable one - so when the last thread completes, its uthread struct and 32 KB stack sit in the list until something calls uthread_schedule() again. Nothing does, so each usb_init() leaves one thread behind. Call uthread_schedule() once more after the loop exits to drain the tail. Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/usb/host/usb-uclass.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index a7ac18a7a83..c2c6b79efde 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -375,6 +375,12 @@ static void run_threads(void) return; while (!uthread_grp_done(grp_id)) uthread_schedule(); + /* + * uthread_schedule() only frees 'done' threads it walks past + * on the way to the next runnable one. Call it again so it + * drains the tail of the list and releases the last thread. + */ + uthread_schedule(); nthr = 0; grp_id = 0; #endif -- 2.43.0
From: Simon Glass <sjg@chromium.org> part_create_block_devices() allocates a name for the block-partition driver, but device_bind_driver() does not set DM_FLAG_NAME_ALLOCED, so the name is not released when the partition device is unbound. Every scanned partition thus leaks its name. Call device_set_name_alloced() so the core frees it on unbind. Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/block/blk-uclass.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c index 9c9169d6ac8..611859435f2 100644 --- a/drivers/block/blk-uclass.c +++ b/drivers/block/blk-uclass.c @@ -808,6 +808,7 @@ static int part_create_block_devices(struct udevice *blk_dev) strdup(devname), &dev); if (ret) return ret; + device_set_name_alloced(dev); part_data = dev_get_uclass_plat(dev); part_data->partnum = part; -- 2.43.0
From: Simon Glass <sjg@chromium.org> At present common_test_malloc_very_large() asks for what it thinks should be available (TOTAL_MALLOC_LEN - margin) in a single malloc() call. Similarly common_test_malloc_fill_pool() asserts that the running total reaches (TOTAL_MALLOC_LEN - 1 MiB). Both assume the whole pool is a single contiguous region. When earlier tests in the same sandbox session have left scattered leaks the heap is fragmented, and either test can fail even though the help is mostly empty. Use malloc_largest_free() instead so the two tests scale down to what is actually available, and allocate half of that when checking that a large block can still be obtained after the fill-and-free cycle. The tests still exercise the intended behaviour without depending on prior tests not leaking anything. Signed-off-by: Simon Glass <sjg@chromium.org> --- test/common/malloc.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/test/common/malloc.c b/test/common/malloc.c index c4764a51f6b..198258b5c89 100644 --- a/test/common/malloc.c +++ b/test/common/malloc.c @@ -545,15 +545,17 @@ static int common_test_malloc_very_large(struct unit_test_state *uts) before = get_alloced_size(); /* - * When mcheck is enabled, it adds overhead per allocation (header + - * canaries). With large CONFIG_MCHECK_CALLER_LEN, this can be - * significant. Use a larger margin to account for mcheck overhead. + * Target the largest free chunk rather than the whole pool, so earlier + * test leaks that fragment the heap do not turn this into a flakily + * failing test. When mcheck is enabled, allow for the per-allocation + * header and canaries; otherwise a smaller margin for dlmalloc's own + * chunk alignment is enough. */ if (CONFIG_IS_ENABLED(MCHECK_HEAP_PROTECTION)) margin = SZ_256K; else margin = SZ_64K; - size = TOTAL_MALLOC_LEN - before - margin; + size = malloc_largest_free() - margin; ptr = malloc(size); ut_assertnonnull(ptr); @@ -582,6 +584,7 @@ static int common_test_malloc_fill_pool(struct unit_test_state *uts) { int alloc_size, before, count, i, total; const int ptr_table_size = 0x100000; + size_t largest; void **ptrs; void *ptr; @@ -594,6 +597,14 @@ static int common_test_malloc_fill_pool(struct unit_test_state *uts) before = get_alloced_size(); + /* + * Record the largest contiguous free region up front. Earlier tests + * may have left scattered leaks that fragment the heap, so cap the + * assertion below against what is actually available rather than the + * whole pool. + */ + largest = malloc_largest_free(); + /* Use memory outside malloc pool to store pointers */ ptrs = map_sysmem(0x1000, ptr_table_size); @@ -616,10 +627,10 @@ static int common_test_malloc_fill_pool(struct unit_test_state *uts) ptr_table_size); /* - * Should have allocated most of the pool - if we can't allocate 1MB, - * then at most 1MB is available, so we must have allocated at least - * (pool_size - 1MB). Save the peak before freeing so an assertion - * failure does not leak the entire pool. + * Should have allocated most of the available pool - if we can't + * allocate 1 MB, then at most 1 MB is available, so we must have + * allocated at least (available - 1 MB). Save the peak before + * freeing so an assertion failure does not leak the entire pool. */ ut_assert(count > 0); ut_assert(count < ptr_table_size / sizeof(void *)); @@ -632,13 +643,13 @@ static int common_test_malloc_fill_pool(struct unit_test_state *uts) for (i = 0; i < count; i++) free(ptrs[i]); - ut_assert(alloc_size >= TOTAL_MALLOC_LEN - SZ_1M); + ut_assert(alloc_size - before >= largest - SZ_1M); /* Should be back to starting state */ ut_asserteq(before, get_alloced_size()); - /* Verify we can allocate large blocks again */ - ptr = malloc(TOTAL_MALLOC_LEN / 2); + /* Verify we can allocate a large block again */ + ptr = malloc(largest / 2); ut_assertnonnull(ptr); free(ptr); -- 2.43.0
From: Simon Glass <sjg@chromium.org> dm_test_pre_run() sets gd->dm_root to NULL and calls dm_init(), which resets the uclass list head via INIT_LIST_HEAD(). That abandons the global driver-model state built by sandbox at boot without freeing it, leaving every uclass and device orphaned in the heap. At the end of each ut_run_list(), dm_test_restore() rebuilds a fresh global state with another dm_init() + dm_scan_plat() + dm_extended_scan(), which the next test's dm_test_pre_run() also orphans. Each 'ut' command therefore leaks several hundred allocations, and a long pytest session eventually exhausts the mcheck registry. The documented intent (see comment in test_pre_run()) is that UTF_DM tests run against a scratch state and leave the global state untouched. Implement that: snapshot gd->dm_root, gd->uclass_root_s, and gd->uclass_root before wiping them in dm_test_pre_run(), and write the snapshot back at the end of dm_test_post_run() once the test's uclasses have been destroyed. The saved uclasses' sibling_node pointers still reference the same &gd->uclass_root_s address, so restoring the list head re-links the list without any fix-up. The uclass_root pointer itself must also be restored since tests like dm_test_uclass_before_ready zero it explicitly. dm_test_restore() no longer needs to rebuild anything and only resets the live-tree root. Signed-off-by: Simon Glass <sjg@chromium.org> --- include/test/test.h | 10 ++++++++++ test/test-main.c | 40 +++++++++++++++++++++++++++++++--------- 2 files changed, 41 insertions(+), 9 deletions(-) diff --git a/include/test/test.h b/include/test/test.h index 7d28948eb5f..b4b20f6a701 100644 --- a/include/test/test.h +++ b/include/test/test.h @@ -9,6 +9,7 @@ #include <abuf.h> #include <malloc.h> #include <linux/bitops.h> +#include <linux/list.h> #define UT_MAX_ARGS 8 #define UT_PRIV_SIZE 256 @@ -78,6 +79,12 @@ struct ut_arg { * @start: Store the starting mallinfo when doing leak test * @of_live: true to use livetree if available, false to use flattree * @of_root: Record of the livetree root node (used for setting up tests) + * @saved_dm_root: Global dm_root swapped out while a UTF_DM test runs + * @saved_uclass_root: Global uclass-root list head swapped out while a + * UTF_DM test runs, so the pre-test state can be restored afterwards + * @saved_uclass_root_ptr: Saved value of gd->uclass_root (the pointer to + * the uclass-root list head). Restored alongside the list head above + * in case a test zeros the pointer (e.g. dm_test_uclass_before_ready) * @root: Root device * @testdev: Test device * @force_fail_alloc: Force all memory allocs to fail @@ -115,6 +122,9 @@ struct unit_test_state { int worst_ms; struct mallinfo start; struct device_node *of_root; + struct udevice *saved_dm_root; + struct list_head saved_uclass_root; + struct list_head *saved_uclass_root_ptr; bool of_live; struct udevice *root; struct udevice *testdev; diff --git a/test/test-main.c b/test/test-main.c index 8bbe7d039bc..fc66b2af0d0 100644 --- a/test/test-main.c +++ b/test/test-main.c @@ -263,7 +263,20 @@ static int dm_test_pre_run(struct unit_test_state *uts) if (fdt_action() == FDTCHK_CHECKSUM) uts->fdt_chksum = crc8(0, gd->fdt_blob, fdt_totalsize(gd->fdt_blob)); + + /* + * Save the global driver-model state so it can be restored after the + * test. Snapshot the uclass list head and the pointer to it, and + * detach the existing list. The old uclasses are left intact in + * memory but are no longer reachable, so a fresh dm_init() can build + * up a private list for the test without disturbing them. + */ + uts->saved_dm_root = gd->dm_root; + uts->saved_uclass_root = gd->uclass_root_s; + uts->saved_uclass_root_ptr = gd->uclass_root; gd->dm_root = NULL; + INIT_LIST_HEAD(&gd->uclass_root_s); + malloc_disable_testing(); if (CONFIG_IS_ENABLED(UT_DM) && !CONFIG_IS_ENABLED(OF_PLATDATA)) memset(dm_testdrv_op_count, '\0', sizeof(dm_testdrv_op_count)); @@ -330,6 +343,19 @@ static int dm_test_post_run(struct unit_test_state *uts) } } + /* + * Restore the global driver-model state that was saved by + * dm_test_pre_run(). Writing the saved list_head back reconnects + * the saved list because the uclasses' sibling_node pointers still + * reference &gd->uclass_root_s at the same address. A test may have + * zeroed gd->uclass_root (e.g. dm_test_uclass_before_ready) so put + * the pointer back as well. + */ + gd->dm_root = uts->saved_dm_root; + gd->uclass_root_s = uts->saved_uclass_root; + gd->uclass_root = uts->saved_uclass_root_ptr; + uts->saved_dm_root = NULL; + return 0; } @@ -434,21 +460,17 @@ static bool ut_list_has_dm_tests(struct unit_test *tests, int count, /** * dm_test_restore() Put things back to normal so sandbox works as expected * + * dm_test_pre_run()/dm_test_post_run() save and restore the global driver + * model state across each UTF_DM test, so the global root is already back + * in place by the time we get here. Only restore the live-tree root, which + * per-test setup leaves pointing at the test's own tree. + * * @of_root: Value to set for of_root * Return: 0 if OK, -ve on error */ static int dm_test_restore(struct device_node *of_root) { - int ret; - gd_set_of_root(of_root); - gd->dm_root = NULL; - ret = dm_init(CONFIG_IS_ENABLED(OF_LIVE)); - if (ret) - return ret; - dm_scan_plat(false); - if (!CONFIG_IS_ENABLED(OF_PLATDATA)) - dm_extended_scan(false); return 0; } -- 2.43.0
From: Simon Glass <sjg@chromium.org> dm_test_video_sync_damage() sets video_set_manual_sync(true) so that its assertions can observe the damage rectangle in isolation from automatic syncs. The mirror test dm_test_video_manual_sync() puts the flag back to false on exit, but this one falls out of the function with the flag still set. Any later test that relies on video_sync() actually copying fb to copy_fb then fails spuriously. Reset the flag before returning, matching dm_test_video_manual_sync so the two tests can run in any order and leave the global video state as they found it. Signed-off-by: Simon Glass <sjg@chromium.org> --- test/dm/video.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/dm/video.c b/test/dm/video.c index f97e2183a64..18d257e39f4 100644 --- a/test/dm/video.c +++ b/test/dm/video.c @@ -1618,6 +1618,9 @@ static int dm_test_video_sync_damage(struct unit_test_state *uts) /* Check priv->damage after sync - should be reset to inverted/empty */ ut_assert(!vid_bbox_valid(&priv->damage)); + /* Restore auto-sync so later tests see the default behaviour */ + video_set_manual_sync(false); + return 0; } DM_TEST(dm_test_video_sync_damage, UTF_SCAN_PDATA | UTF_SCAN_FDT); -- 2.43.0
From: Simon Glass <sjg@chromium.org> dm_test_video_silence() compares the framebuffer against an exact compressed byte count, which only matches when the vidconsole is using its default font. If an earlier test (for example expo or cedit) switched the font and did not restore it, the messages that this test writes rasterise to different pixels and the byte count no longer matches. Select the default font before clearing and writing, so the test does its work with a known font regardless of what state a preceding test left behind. Signed-off-by: Simon Glass <sjg@chromium.org> --- test/dm/video.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/dm/video.c b/test/dm/video.c index 18d257e39f4..a0608ebc23f 100644 --- a/test/dm/video.c +++ b/test/dm/video.c @@ -1214,6 +1214,12 @@ static int dm_test_video_silence(struct unit_test_state *uts) sdev = stdio_get_by_name("vidconsole"); ut_assertnonnull(sdev); con = sdev->priv; + /* + * Restore the default font so an earlier test that left a different + * one selected does not change the pixels written by the messages + * below. + */ + ut_assertok(vidconsole_select_font(con, NULL, NULL, 0)); ut_assertok(vidconsole_clear_and_reset(con)); ut_unsilence_console(uts); -- 2.43.0
From: Simon Glass <sjg@chromium.org> Running bootctl_logic_tkey before dm_test_usb_stop causes the latter to fail: uclass_get_device(UCLASS_MASS_STORAGE, 0, ...) returns -ENODEV and count_usb_devices() returns 2 instead of 7. The bootctl test scans USB during setup, which sets the global usb_started flag. When the test ends, dm_test_post_run() destroys every uclass, removing the USB devices, but usb_started stays true. The usb_stop test has UTF_SCAN_PDATA | UTF_SCAN_FDT rather than UTF_DM, so no driver-model reset happens before it runs, and its usb_init() call short-circuits on the stale flag without scanning any buses. Clear usb_started at the start of the test so usb_init() performs a real scan regardless of what state an earlier test left behind. Signed-off-by: Simon Glass <sjg@chromium.org> --- test/dm/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/dm/usb.c b/test/dm/usb.c index 66777b5ef00..b3cf8ce8c36 100644 --- a/test/dm/usb.c +++ b/test/dm/usb.c @@ -152,6 +152,14 @@ static int dm_test_usb_stop(struct unit_test_state *uts) { struct udevice *dev; + /* + * A previous test (e.g. bootctl_logic_tkey) may have left the + * global usb_started flag set while the USB uclass was then + * destroyed by dm_test_post_run(). Reset it here so usb_init() + * actually scans the buses, rather than short-circuiting. + */ + usb_started = false; + /* Scan and check that all devices are present */ state_set_skip_delays(true); ut_assertok(usb_init()); -- 2.43.0
From: Simon Glass <sjg@chromium.org> bootdev_find_in_blk() calls fs_set_blk_dev_with_part() to probe the partition and leaves the mount active for the bootmeth. Most bootmeths call fs_size()/fs_read() (which close the fs via fs_legacy), and those that do their own lookups re-mount via bootmeth_setup_fs(). But every call chain ends up with one mount still open - nothing calls fs_close() to release it. For ext4 that leaks the jbd2 journal, revoke tables and mballoc state every time a partition is scanned - hundreds of KB across the test suite. Call fs_close() after bootmeth_read_bootflow(). It is a no-op if the fs was already closed (fs_type reset to FS_TYPE_ANY). Signed-off-by: Simon Glass <sjg@chromium.org> --- boot/bootdev-uclass.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/boot/bootdev-uclass.c b/boot/bootdev-uclass.c index 72f9a315c04..faed1f3e5df 100644 --- a/boot/bootdev-uclass.c +++ b/boot/bootdev-uclass.c @@ -191,6 +191,15 @@ int bootdev_find_in_blk(struct udevice *dev, struct udevice *blk, log_debug("using method %s\n", bflow->method->name); ret = bootmeth_read_bootflow(bflow->method, bflow); log_debug("method %s returned ret=%d\n", bflow->method->name, ret); + + /* + * Close any filesystem left mounted by the bootmeth. This is a no-op + * if the last fs operation already closed, but ensures a dangling + * mount (e.g. from fs_set_blk_dev_with_part() above when the + * bootmeth performed no fs_size()/fs_read()) is released. + */ + fs_close(); + if (ret) return log_msg_ret("method", ret); -- 2.43.0
From: Simon Glass <sjg@chromium.org> script_read_bootflow_file() reads boot.bmp into a local abuf and then moves the pointer into bflow->logo with abuf_uninit_move() The logo buffer is allocated but bootflow_free() never frees it, so every scanned bootflow that has a logo leaks the whole image. Free bflow->logo alongside the other bootflow fields. Signed-off-by: Simon Glass <sjg@chromium.org> --- boot/bootflow.c | 1 + 1 file changed, 1 insertion(+) diff --git a/boot/bootflow.c b/boot/bootflow.c index ff98cc4aa56..28502c309b6 100644 --- a/boot/bootflow.c +++ b/boot/bootflow.c @@ -754,6 +754,7 @@ void bootflow_free(struct bootflow *bflow) free(bflow->fname); if (!(bflow->flags & BOOTFLOWF_STATIC_BUF)) free(bflow->buf); + free(bflow->logo); free(bflow->os_name); free(bflow->fdt_fname); /* bootmeth_priv is only set when method is set */ -- 2.43.0
From: Simon Glass <sjg@chromium.org> The bootctrl_ui uclass allocates a bc_ui_priv per device, and UI drivers (multiboot_ui, simple_ui) populate upriv->expo via bootflow_menu_setup() or build their own via expo_new() Neither is freed when the device goes away, so the expo and every scene, string and object hung off it are leaked - about 50 KB per logic_tkey run. Add a pre_remove hook on the uclass to destroy the expo and release the autoboot_template abuf. Signed-off-by: Simon Glass <sjg@chromium.org> --- boot/bootctl/bootctl-uclass.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/boot/bootctl/bootctl-uclass.c b/boot/bootctl/bootctl-uclass.c index 756702b3e9a..3f06668a199 100644 --- a/boot/bootctl/bootctl-uclass.c +++ b/boot/bootctl/bootctl-uclass.c @@ -8,8 +8,10 @@ #define LOG_CATEGORY UCLASS_BOOTCTL +#include <abuf.h> #include <bootctl.h> #include <dm.h> +#include <expo.h> #include <bootctl/ui.h> UCLASS_DRIVER(bootctrl) = { @@ -39,9 +41,21 @@ UCLASS_DRIVER(bootctrl_state) = { .per_device_plat_auto = sizeof(struct bootctl_uc_plat), }; +static int bootctl_ui_pre_remove(struct udevice *dev) +{ + struct bc_ui_priv *upriv = dev_get_uclass_priv(dev); + + if (upriv->expo) + expo_destroy(upriv->expo); + abuf_uninit(&upriv->autoboot_template); + + return 0; +} + UCLASS_DRIVER(bootctrl_ui) = { .id = UCLASS_BOOTCTL_UI, .name = "bootctrl_ui", + .pre_remove = bootctl_ui_pre_remove, .per_device_plat_auto = sizeof(struct bootctl_uc_plat), .per_device_auto = sizeof(struct bc_ui_priv), }; -- 2.43.0
From: Simon Glass <sjg@chromium.org> The simple_state driver accumulates key/value pairs in priv->items, each with allocated key and val Nothing releases them when the device is removed, so a test that calls write_val() repeatedly leaks hundreds of entries. Add a remove() method to call clear_vals() and tear down the alist. Signed-off-by: Simon Glass <sjg@chromium.org> --- boot/bootctl/simple_state.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/boot/bootctl/simple_state.c b/boot/bootctl/simple_state.c index 062bb697fbd..84da799e84a 100644 --- a/boot/bootctl/simple_state.c +++ b/boot/bootctl/simple_state.c @@ -435,6 +435,16 @@ static const struct udevice_id sstate_ids[] = { { } }; +static int sstate_remove(struct udevice *dev) +{ + struct sstate_priv *priv = dev_get_priv(dev); + + clear_vals(priv); + alist_uninit(&priv->items); + + return 0; +} + U_BOOT_DRIVER(simple_state) = { .name = "simple_state", .id = UCLASS_BOOTCTL_STATE, @@ -442,6 +452,7 @@ U_BOOT_DRIVER(simple_state) = { .ops = &ops, .bind = sstate_bind, .probe = sstate_probe, + .remove = sstate_remove, .of_to_plat = sstate_of_to_plat, .priv_auto = sizeof(struct sstate_priv), }; -- 2.43.0
From: Simon Glass <sjg@chromium.org> The 'cedit load' command builds a new expo and stashes the pointer in cur_exp, overwriting any previous one without freeing it. When 'cedit run' is successful, the expo is destroyed, but on failure (or when a test inspects cur_exp and never calls 'cedit run' again) the previous expo lingers and leaks every scene, string and object it owns, about 20 KB per cedit test. Destroy any current expo before replacing it on reload. Signed-off-by: Simon Glass <sjg@chromium.org> --- cmd/cedit.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cmd/cedit.c b/cmd/cedit.c index fc477291be8..bcd75fdb771 100644 --- a/cmd/cedit.c +++ b/cmd/cedit.c @@ -62,6 +62,8 @@ static int do_cedit_load(struct cmd_tbl *cmdtp, int flag, int argc, return CMD_RET_FAILURE; } + if (cur_exp) + expo_destroy(cur_exp); cur_exp = exp; return 0; -- 2.43.0
From: Simon Glass <sjg@chromium.org> cedit_run() calls expo_enter_mode() which sets video_set_manual_sync() to true, pairing with expo_exit_mode() which clears it again. Error paths inside the loop call expo_exit_mode() before returning, but the normal exit (exp->done becoming true) falls through to the bottom of the function and skips it, so when the user quits without saving we return -EACCES with manual-sync still active. Any later code that relies on video_sync() to copy fb to copy_fb then silently does nothing. The dm_test_video_silence test sees this as fb != copy_fb after its own video_sync and fails, purely because an earlier test exercised cedit. Call expo_exit_mode() once the loop has finished, before evaluating the save/return paths, so the sync flag is always restored. Signed-off-by: Simon Glass <sjg@chromium.org> --- boot/cedit.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/boot/cedit.c b/boot/cedit.c index f047849e19d..7dc369008ee 100644 --- a/boot/cedit.c +++ b/boot/cedit.c @@ -268,6 +268,8 @@ int cedit_run(struct expo *exp) } } while (!exp->done); + expo_exit_mode(exp); + if (ret) return log_msg_ret("end", ret); if (!exp->save) -- 2.43.0
From: Simon Glass <sjg@chromium.org> do_upl_write() builds a live tree via upl_create_handoff_tree() and flattens it with oftree_to_fdt(), but never disposes the tree. Similarly, do_upl_read() calls oftree_from_fdt() and drops the resulting tree on return. Each invocation of 'upl read'/'upl write' therefore leaks several kilobytes of ofnode state. Dispose of both trees once they are no longer needed. Signed-off-by: Simon Glass <sjg@chromium.org> --- cmd/upl.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cmd/upl.c b/cmd/upl.c index ef2183d8528..c6748fa1f7f 100644 --- a/cmd/upl.c +++ b/cmd/upl.c @@ -68,6 +68,7 @@ static int do_upl_write(struct cmd_tbl *cmdtp, int flag, int argc, log_debug("Flattening\n"); ret = oftree_to_fdt(tree, &buf); + oftree_dispose(tree); if (ret) { log_err("Failed to write (err=%dE)\n", ret); return CMD_RET_FAILURE; @@ -100,6 +101,7 @@ static int do_upl_read(struct cmd_tbl *cmdtp, int flag, int argc, printf("Reading UPL at %lx\n", addr); tree = oftree_from_fdt(map_sysmem(addr, 0)); ret = upl_read_handoff(upl, tree); + oftree_dispose(tree); if (ret) { log_err("Failed to read (err=%dE)\n", ret); return CMD_RET_FAILURE; -- 2.43.0
From: Simon Glass <sjg@chromium.org> This dm_test_oftree_new() test unflattens a secondary device tree using oftree_from_fdt() but never frees it, thus leaking about 6K of memory per run. Dispose of the tree before returning. Signed-off-by: Simon Glass <sjg@chromium.org> --- test/dm/ofnode.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/dm/ofnode.c b/test/dm/ofnode.c index 0f60c2a6281..516e24ceb28 100644 --- a/test/dm/ofnode.c +++ b/test/dm/ofnode.c @@ -1492,6 +1492,8 @@ static int dm_test_livetree_ensure(struct unit_test_state *uts) ut_asserteq_str("sandbox-other2", ofnode_read_string(node, "compatible")); + oftree_dispose(tree); + return 0; } DM_TEST(dm_test_livetree_ensure, UTF_SCAN_FDT); -- 2.43.0
From: Simon Glass <sjg@chromium.org> The oftree_new test allocates a live tree via oftree_new() and never disposes of it. Release the root on exit so the test-local tree is cleaned up. Note: of_live_free() currently only frees the root node, so subnodes added via ofnode_add_subnode() still leak. We'll deal with that later. Signed-off-by: Simon Glass <sjg@chromium.org> --- test/dm/ofnode.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/dm/ofnode.c b/test/dm/ofnode.c index 516e24ceb28..838a216cfa9 100644 --- a/test/dm/ofnode.c +++ b/test/dm/ofnode.c @@ -1510,6 +1510,8 @@ static int dm_test_oftree_new(struct unit_test_state *uts) check = ofnode_find_subnode(node, "edmund"); ut_asserteq(check.of_offset, subnode.of_offset); + oftree_dispose(tree); + return 0; } DM_TEST(dm_test_oftree_new, UTF_SCAN_FDT); -- 2.43.0
From: Simon Glass <sjg@chromium.org> This test registers up to OFNODE_MULTI_TREE_MAX live trees, in a loop, to hit the -EOVERFLOW path, but never calls free_oftree() on the successfully-registered ones. Each leaks several KB per run. Free each tree after the check. Signed-off-by: Simon Glass <sjg@chromium.org> --- test/dm/ofnode.c | 1 + 1 file changed, 1 insertion(+) diff --git a/test/dm/ofnode.c b/test/dm/ofnode.c index 838a216cfa9..3fd3b8509f9 100644 --- a/test/dm/ofnode.c +++ b/test/dm/ofnode.c @@ -1379,6 +1379,7 @@ static int dm_test_ofnode_too_many(struct unit_test_state *uts) */ if (of_live_active() || i < max_trees - 1) { ut_assertok(ret); + free_oftree(tree); } else { /* * tree should be invalid when we try to register too -- 2.43.0
From: Simon Glass <sjg@chromium.org> This test allocates two abufs via oftree_to_fdt() and a live tree via oftree_from_fdt(), but never frees them, leaking about 200K each time the test runs. Dispose of the live tree and uninit the abufs at the end of the test. Signed-off-by: Simon Glass <sjg@chromium.org> --- test/dm/ofnode.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/dm/ofnode.c b/test/dm/ofnode.c index 3fd3b8509f9..68e436da929 100644 --- a/test/dm/ofnode.c +++ b/test/dm/ofnode.c @@ -1629,6 +1629,10 @@ static int dm_test_oftree_to_fdt(struct unit_test_state *uts) ut_asserteq(abuf_size(&buf), abuf_size(&buf2)); ut_asserteq_mem(abuf_data(&buf), abuf_data(&buf2), abuf_size(&buf)); + oftree_dispose(check); + abuf_uninit(&buf); + abuf_uninit(&buf2); + return 0; } DM_TEST(dm_test_oftree_to_fdt, UTF_SCAN_FDT); -- 2.43.0
From: Simon Glass <sjg@chromium.org> This test builds two live trees via upl_create_handoff_tree() and oftree_from_fdt() but never disposes them, leaking most of the node and property allocations on each run. Dispose of both before returning. Signed-off-by: Simon Glass <sjg@chromium.org> --- test/boot/upl.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/boot/upl.c b/test/boot/upl.c index e2dc3d51eda..0730f102a78 100644 --- a/test/boot/upl.c +++ b/test/boot/upl.c @@ -352,6 +352,8 @@ static int upl_test_base(struct unit_test_state *uts) ut_assertok(upl_read_handoff(&check, check_tree)); ut_assertok(compare_upl(uts, &upl, &check)); + oftree_dispose(check_tree); + oftree_dispose(tree); abuf_uninit(&buf); return 0; -- 2.43.0
From: Simon Glass <sjg@chromium.org> This function creates a live tree via oftree_from_fdt() but never disposes if it, leaking ~2.7K of ofnode state. Dispose of the tree before returning. Signed-off-by: Simon Glass <sjg@chromium.org> --- test/lib/json.c | 1 + 1 file changed, 1 insertion(+) diff --git a/test/lib/json.c b/test/lib/json.c index 0cddddcb2a3..1c5c9215cc3 100644 --- a/test/lib/json.c +++ b/test/lib/json.c @@ -539,6 +539,7 @@ static int lib_test_json_to_fdt_luks2(struct unit_test_state *uts) /* Check keyslots_size (stored as string in JSON) */ ut_asserteq_str("3145728", ofnode_read_string(config, "keyslots_size")); + oftree_dispose(tree); abuf_uninit(&buf); return 0; -- 2.43.0
From: Simon Glass <sjg@chromium.org> This test uses driver model so should use the UTF_DM flag, to avoid leaking 1MB for the MMC backing-buffers. Signed-off-by: Simon Glass <sjg@chromium.org> --- test/boot/bootflow.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index 0a17f12b6a4..c4be33a63c2 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -1303,7 +1303,7 @@ static int bootflow_cmdline(struct unit_test_state *uts) return 0; } -BOOTSTD_TEST(bootflow_cmdline, UTF_CONSOLE); +BOOTSTD_TEST(bootflow_cmdline, UTF_DM | UTF_SCAN_FDT | UTF_CONSOLE); /* test a few special changes to a long command line */ static int bootflow_cmdline_special(struct unit_test_state *uts) -- 2.43.0
From: Simon Glass <sjg@chromium.org> Like bootflow_cmdline(), bootstd_images() scans for bootflows and binds extra MMC nodes but declares itself with only UTF_CONSOLE, so the DM teardown path never runs. This leaks around 55KB of device state per run. Add UTF_DM and UTF_SCAN_FDT to match the other bootflow tests. Signed-off-by: Simon Glass <sjg@chromium.org> --- test/boot/bootflow.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index c4be33a63c2..9648ecb7b99 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -1662,7 +1662,7 @@ static int bootstd_images(struct unit_test_state *uts) return 0; } -BOOTSTD_TEST(bootstd_images, UTF_CONSOLE); +BOOTSTD_TEST(bootstd_images, UTF_DM | UTF_SCAN_FDT | UTF_CONSOLE); /* Check creation of ad-hoc images */ static int bootstd_adhoc(struct unit_test_state *uts) -- 2.43.0
From: Simon Glass <sjg@chromium.org> Like bootflow_cmdline() and bootstd_images(), bootstd_adhoc() loads from MMC but does not enable UTF_DM so nothing unwinds the probed devices afterwards. Add UTF_DM and UTF_SCAN_FDT to match the other tests. Note: bootflow_efi() has the same issue, but we cannot enable UTF_DM in that test, since it causes memory corruption, due to a conflict between the lifetimes of the EFI subsystem and driver model. This is left for a future effort. Signed-off-by: Simon Glass <sjg@chromium.org> --- test/boot/bootflow.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index 9648ecb7b99..a2a0a7eeb1e 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -1682,7 +1682,7 @@ static int bootstd_adhoc(struct unit_test_state *uts) return 0; } -BOOTSTD_TEST(bootstd_adhoc, UTF_CONSOLE); +BOOTSTD_TEST(bootstd_adhoc, UTF_DM | UTF_SCAN_FDT | UTF_CONSOLE); /* Check scanning extlinux, adjusting cmdline and booting */ static int bootflow_scan_extlinux(struct unit_test_state *uts) -- 2.43.0
From: Simon Glass <sjg@chromium.org> This test uses 'setexpr.s' to set the env variable 'fred' to a 64K string, but never unsets it. The env machinery allocated the value, so running the test leaves 64K hanging off the env hash forever. Unset 'fred' at the end of the test. Signed-off-by: Simon Glass <sjg@chromium.org> --- test/cmd/setexpr.c | 1 + 1 file changed, 1 insertion(+) diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c index 93b0c4b68f5..4e3a2f7aaa4 100644 --- a/test/cmd/setexpr.c +++ b/test/cmd/setexpr.c @@ -360,6 +360,7 @@ static int setexpr_test_str_long(struct unit_test_state *uts) ut_asserteq(64 << 10, strlen(val)); unmap_sysmem(buf); + env_set("fred", NULL); return 0; } -- 2.43.0
From: Simon Glass <sjg@chromium.org> bootflow_efi and similar tests run 'bootflow select N', which stores a pointer to an entry of std->bootflows in std->cur_bootflow. The bootflow alist holds its elements in one allocated buffer and reallocs that buffer (dropping the old one, which mcheck flood-fills with 0xf5) when the list grows. A later test can then re-grow the alist, leaving cur_bootflow dangling. ut_measurement_measure hits this: env_set("bootargs", ...) fires the on_bootargs callback which reads std->cur_bootflow, tries to free bflow->cmdline, and segfaults on the flood-filled pointer. Reset cur_bootflow to NULL in test_post_run() so no test leaves a dangling reference behind. on_bootargs already returns early when cur_bootflow is NULL, so the callback becomes a no-op instead of a use-after-free. Signed-off-by: Simon Glass <sjg@chromium.org> --- test/test-main.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/test-main.c b/test/test-main.c index fc66b2af0d0..a8ec76ab8ad 100644 --- a/test/test-main.c +++ b/test/test-main.c @@ -8,6 +8,7 @@ #include <blk.h> #include <bootstage.h> +#include <bootstd.h> #include <console.h> #include <cyclic.h> #include <dm.h> @@ -608,9 +609,24 @@ static int test_pre_run(struct unit_test_state *uts, struct unit_test *test) */ static int test_post_run(struct unit_test_state *uts, struct unit_test *test) { + struct bootstd_priv *std; + ut_unsilence_console(uts); if (test->flags & UTF_DM) ut_assertok(dm_test_post_run(uts)); + + /* + * Drop any reference to the currently selected bootflow. The bootflow + * may be inside an alist buffer that a later test re-grows (and + * therefore frees); leaving the pointer behind turns the on_bootargs + * env callback into a use-after-free. + */ + if (CONFIG_IS_ENABLED(BOOTSTD)) { + std = bootstd_try_priv(); + if (std) + std->cur_bootflow = NULL; + } + ut_assertok(cyclic_unregister_all()); ut_assertok(event_uninit()); -- 2.43.0
participants (1)
-
Simon Glass