Re: [PATCH] hw/misc/npcm_clk: fix buffer-overflow

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 2/26/25 03:50, Peter Maydell wrote:
> On Tue, 25 Feb 2025 at 20:57, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
>>
>> On 2/25/25 05:41, Peter Maydell wrote:
>>> (Looking more closely at the cold_reset_values handling
>>> in npcm_gcr.c, that looks not quite right in a different
>>> way; I'll send a reply to that patch email about that.)
>>>
>>
>> It may be a hole in our CI right now.
>> Would that be interesting for CI to run all tests (check-functional +
>> check w/o functional) with both ubsan and asan?
> 
> We do have at least some ubsan tests in our CI right now
> (eg the "clang-system" job). The problem with ubsan coverage
> is the usual one that we already have too much CI going on,
> and it takes forever and we don't have that much headroom
> for adding more jobs.

I understand the problem behind spending more minutes on this.

However, looking at our CI, we already duplicate functional testing a lot:
buildtest.yml:functional-system-alpine:
buildtest.yml:functional-system-ubuntu:
buildtest.yml:functional-system-debian:
buildtest.yml:functional-system-fedora:
buildtest.yml:functional-system-centos:
buildtest.yml:functional-system-opensuse:

Would that hurt so much to have one configuration enabled with ubsan and 
asan, which catches *real* bugs, and potential security issues?
Yes, it adds overhead, but it should not be x10. Around x2 to x3.

On github running, running -j2, running all functional tests with 
sanitizers takes less than 1 hour, and the build takes the same amount 
in time (-j2 as well). Hopefully we have more cores available on our own 
runners.

> 
> On the asan front, also, yes, coverage would be a good idea.
> Here I think we will probably have to gradually ratchet
> up the coverage because I'm pretty sure that at the moment
> we will find we don't get a clean pass (mostly for "uninteresting"
> memory leaks).
> 

Yes, I run with ASAN_OPTIONS=detect_leaks=0, and I deactivate any test 
that is flaky.

Two of them related to asan are tcg tests:
- munmap-pthread
- follow-fork-mode
I didn't have time to investigate, so I just removed them in my tree.

At this point, this whole list of tests concerned is:
https://github.com/search?q=repo%3Apbo-linaro%2Fqemu-ci+%22ci+fix%22+author%3Apbo-linaro&type=commits

> (I do also usually run a local
> ubsan test build when doing my acculumation of patches in
> target-arm, but since that's a manual step it is fallible :-))
>

It's always said that "Maintainer time is precious", shouldn't that be 
CI job to catch this?
I guess CI minutes are cheaper than engineer ones those days.

> -- PMM