CI: add ASAN, UBSAN

[thesamesam]

Signed-off-by: Sam James sam@gentoo.org

[thesamesam]

meh. I can't hit the ASAN failure locally yet. :(

[grobian]

You mean the one in qmanifest? From a quick look it seems to happen with openmp only. I need to look at it later when I have some time to see what it complains about. With some luck I can fix it.

Unrelated: I believe there's also a valgrind run, I'm assuming that won't also run with *SAN, right? Wouldn't make much sense I guess :)

[bstaletic]

Looking at one of the UBSAN errors, the call stack looks like the thing I have come across in #26 !

I have also seen ASAN report a leak, but that does not quite look like the ASAN error in this pull request. I'll be looking into it further.

[bstaletic]

Unfortunately, #26 and #27 are still not enough to satisfy ASAN. One thing remains:

portage-utils/qmanifest.c

Line 1423 in 13b0eec

char *endp = STR + strlen(STR) - 1;\

That line does not consider buf could have an embedded \0 character. I don't know much about gpg, but on my machine, the third line read from here is "\n\x000sh: SHA256\x000\x000 SIGNED MESSAGE-----". Even the previous (2nd) line has some embedded null character.
That means strlen(buf) is 1, while qmanifest has clearly read more than that.

Again, I don't know much about gpg, so I don't even know if the above line is ok-ish.
Here's a godbolt link, reproducing the problem reported by ASAN.
https://godbolt.org/z/qTnKdYGbf

EDIT:

This patch satisfies ASAN, but I don't know if it is correct:

diff --git a/qmanifest.c b/qmanifest.c
index 2bb0f11..d04f743 100644
--- a/qmanifest.c
+++ b/qmanifest.c
@@ -1421,7 +1421,7 @@ verify_manifest(
 #define append_list(STR) \
        if (strncmp(STR, "TIMESTAMP ", 10) != 0 || strncmp(STR, "DIST ", 5) != 0) {\
                char *endp = STR + strlen(STR) - 1;\
-               while (isspace(*endp))\
+               while (isspace(*endp) && endp > STR)\
                        *endp-- = '\0';\
                if (elemslen == elemssize) {\
                        elemssize += LISTSZ;\

EDIT 2:

The above analysis is wrong, but there's still a problem in that append_list(STR) macro. It does not handle lines that contain only whitespace characters, such as "\n". In such cases, *endp-- = '\0' will decrement beyond the start of STR.

I have a few proposals:

1. The patch above, that will leave a single whitespace character, in case the line only has whitespaces.
2. If we want to actually place a \0 in the first character place, we could change the patch above to end >= STR. However, that will still decrement endp beyond begin. In C++ even that is UB, though not sure about C.
3. If we need to avoid that last decrement, the patch becomes uglier:

diff --git a/qmanifest.c b/qmanifest.c
index 2bb0f11..00dfea3 100644
--- a/qmanifest.c
+++ b/qmanifest.c
@@ -1421,8 +1421,11 @@ verify_manifest(
 #define append_list(STR) \
        if (strncmp(STR, "TIMESTAMP ", 10) != 0 || strncmp(STR, "DIST ", 5) != 0) {\
                char *endp = STR + strlen(STR) - 1;\
-               while (isspace(*endp))\
-                       *endp-- = '\0';\
+               while (isspace(*endp) && endp >= STR) {\
+                       *endp = '\0';\
+                       if(endp > STR) --endp;\
+                       if(endp == STR) break;\
+               }\
                if (elemslen == elemssize) {\
                        elemssize += LISTSZ;\
                        elems = xrealloc(elems, elemssize * sizeof(elems[0]));\

EDIT 3:

Maybe strcspn would be useful? OpenBSD man page for fgets recommends strcspn for trimming line feeds.

[grobian]

hey, thanks for your analysis, that's cutting a few corners too much

[grobian]

As far as I understand, your simplest patch is close to the correct one. Instead of endp > STR can just use endp >= STR to make it never consider before the string, but terminate the string into a 0-length one should everything be whitespace.

Doing that is eventually not really relevant because all strings that are compared to are > 1 char, so while at it, I threw in a small optimisation to not bother with anything less than 5 characters. (least: "AUX" + SPACE + 1-char).

[thesamesam]

Once the last PR is in, I'll rebase to get both of your patches in and hopefully it's green then. Thanks a bunch both.

[grobian]

ok, should be pushed now

[bstaletic]

All green! Nice!

[grobian]

Is it useful to use -O3 here? do we get useful stacks and analysis this way? For valgrind we also run with -g -pipe to get understandable complaints

[bstaletic]

A few ideas, since I'm here.

1. ASAN and UBSSAN can be combined, as in -fsanitize=address,undefined. Those are the only two that can be combined, so it might be worth it to reduce the number of CI jobs getting dispatched.
2. Consider enabling MSAN as well, as in -fsanitize=memory. It is a clang-only sanitizer meant to catch reads of uninitialized memory. There are things that are only caught by MSAN: https://godbolt.org/z/TKzxYaEhW