Re: [PATCH v3 0/3] malloc: Enable profiling dlmalloc with valgrind

[Sean Anderson <seanga2@gmail.com>]

Hi all,

On 3/23/22 2:04 PM, Sean Anderson wrote:
> This series adds support for running valgrind against U-Boot's internal
> malloc. This allows for much more useful reports to be generated.
> 
> Some example output of valgrind run against u-boot/master with this branch
> applied may be found at [1]. Note that valgrind gives up around acpi. This
> feature still needs a lot of work on suppressions/hints to filter out the noise
> properly.
> 
> [1] https://gist.githubusercontent.com/Forty-Bot/199bf06f9cdd6871e54f8f484c16e111/raw/2a2f99108eef84b48e27a54332f3f71f4e2e5342/gistfile1.txt

This is a bit of a follow up to the above. Over the past week I've sent

a few patches [1-4] with various bugs found with valgrind. Hopefully

this gives a good idea of the kind of problems valgrind is equipped to

find. In particular, it is good at determining where we go past the end

of buffers or otherwise read uninitialized memory.



I've also discovered why I originally stopped working on this series:

supressions don't "un-taint" uninitialized memory accesses. Currently, I

have dlmalloc's reads its bookkeeping information marked as a "red

zone." This means that all reads to it are marked as illegal by

valgrind. This is fine for regular code, but dlmalloc really does need

to access this area, so we suppress its violations. However, if dlmalloc

then passes a result calculated from a "tainted" access, that result is

still tainted. So the first accessor will raise a warning. This means

that every construct like



	foo = malloc(sizeof(*foo));

	if (!foo)

		return -ENOMEM;



will raise a warning when we check the result of malloc. Whoops.



There are three ways (as I see it) to address this:



- Don't mark dlmalloc bookkeeping information as a red zone. This is the

   simplest solution, but reduces the power of valgrind immensely, since

   we can no longer determine that (e.g.) access past the end of an array

   is undefined.

- Implement red zones properly. This would involve growing every

   allocation by a fixed amount (16 bytes or so) and then using that

   extra space for a real red zone that neither regular code nor dlmalloc

   needs to access. Unfortunately, this would probably some fairly

   intensive surgery to dlmalloc to add/remove the offset appropriately.

- Mark bookkeeping information as valid before we use it in dlmalloc,

   and then mark it invalid before returning. This would be the most

   correct, but it would be very tricky to implement since there are so

   many code paths to mark. I think it would be the most effort out of

   the three options here.



Until one of the above options are implemented, it will remain difficult

to sift through the massive amount of spurious warnings.



That said, if anyone wants to play around with this a bit, I have some

additional invocation suggestions which I have been using, but which are

not documented



- "--error-limit=no" will enable printing more than 1000 errors in a

   single session.

- "--vgdb=yes --vgdb-error=0" will let you use gdb to attach like



	gdb -ex "target remote | vgdb" u-boot



    This is very helpful for inspecting the program state when there is

    an error.

- "t cooked" to U-Boot will keep the console in a sane state if you

   terminate it early (instead of having to run tset).



The above should probably be documented in patch 3. I will add it if I

do a v4 (or I will send a follow-up).



--Sean



[1] https://patchwork.ozlabs.org/project/uboot/list/?series=291719

[2] https://patchwork.ozlabs.org/project/uboot/list/?series=291739

[3] https://patchwork.ozlabs.org/project/uboot/list/?series=292710

[4] https://patchwork.ozlabs.org/project/uboot/list/?series=292712