[PATCH wayland 0/6] Minor test/scanner fixes

[Daniel Stone]

Hi,
These first 5 patches fix some issues I found by running the test suite
under an expanded set of toolchains, including the ASan address
sanitiser and Clang's static analyser.

Patch 6 removes the leak checking from the test suite completely.
Previously due to toolchain immaturity, it was really difficult to run
the test suite with checks for memory leaks, so we rolled our own. On
the other hand, with Meson we can now just pass '-Db_sanitize=address'
to the configure process, or run 'meson test --wrapper=valgrind', to get
more powerful and useful checkers.

The existing leak checker we have breaks ASan completely, and I couldn't
figure out how to fix it. Removing it altogether seemed like a better
idea.

I've implemented this for GitLab CI, and you can see example output
here: https://gitlab.freedesktop.org/daniels/wayland/pipelines/3663

This issue tracks the work left on the CI pipeline to get everything
upstream:
https://gitlab.freedesktop.org/wayland/wayland/issues/54

If anyone wants to help out, please feel free to grab that branch and
run with it; in the meantime, these seem like good fixes to have
regardless.

Cheers,
Daniel

[Daniel Stone]

Help static analysers by letting them know that once we fail(),
execution will terminally complete.

Signed-off-by: Daniel Stone <***@collabora.com>
---
src/scanner.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/scanner.c b/src/scanner.c
index 3afc3d3d..084f196d 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -371,7 +371,7 @@ desc_dump(char *desc, const char *fmt, ...)
putchar('\n');
}

-static void
+static void __attribute__ ((noreturn))
fail(struct location *loc, const char *msg, ...)
{
va_list ap;

[Daniel Stone]

Found with both ASan leak sanitizer and Valgrind. We were trivially
leaking the enum name for every arg parsed by the scanner which had one.
If libxml-based DTD validation was enabled, we would also leak the DTD
itself, despite diligently freeing the document, context, etc.

Signed-off-by: Daniel Stone <***@collabora.com>
---
src/scanner.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/src/scanner.c b/src/scanner.c
index 205c28a9..3afc3d3d 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -133,6 +133,7 @@ is_dtd_valid(FILE *input, const char *filename)
rc = xmlValidateDtd(dtdctx, doc, dtd);
xmlFreeDoc(doc);
xmlFreeParserCtxt(ctx);
+ xmlFreeDtd(dtd);
xmlFreeValidCtxt(dtdctx);
/* xmlIOParseDTD consumes buffer */

@@ -432,6 +433,7 @@ free_arg(struct arg *arg)
free(arg->name);
free(arg->interface_name);
free(arg->summary);
+ free(arg->enumeration_name);
free(arg);
}

[Daniel Stone]

Clang warns that it can silently discard a non-volatile write to a NULL
pointer (perhaps it constitutes undefined behaviour?), and recommends
changing it to volatile.

This patch slavishly complies with the demand of the unfeeling machine.

Signed-off-by: Daniel Stone <***@collabora.com>
---
tests/sanity-test.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/sanity-test.c b/tests/sanity-test.c
index 66ca16fb..2495a115 100644
--- a/tests/sanity-test.c
+++ b/tests/sanity-test.c
@@ -70,8 +70,10 @@ FAIL_TEST(fail_kill)

FAIL_TEST(fail_segv)
{
+ char * volatile *null = 0;
+
test_disable_coredumps();
- * (char **) 0 = "Goodbye, world";
+ *null = "Goodbye, world";
}

FAIL_TEST(sanity_assert)

[Thiago Macieira]

Post by Daniel Stone
Clang warns that it can silently discard a non-volatile write to a NULL
pointer (perhaps it constitutes undefined behaviour?), and recommends

It is.

Post by Daniel Stone
changing it to volatile.

That doesn't help. It's still UB and can be discarded because you're not
allowed to provoke UB. If you need to write to a whose value is identical to
NULL, write assembly.

How about a raise(SIGSEGV) instead?

[Daniel Stone]

libxml2 unconditonally defines XMLCALL to nothing. Expat does not
redefine XMLCALL if it is already defined, but if it is not, and we are
building with gcc on i386 (not x86-64), it will define it as 'cdecl'.

Including Expat before libxml thus results in a warning about XMLCALL
being redefined. Luckily we can get around this by just reversing the
include order: cdecl is a no-op on Unix-like systems, so by having
libxml first define XMLCALL to nothing and including Expat afterwards,
we avoid the warning and lose nothing.

Signed-off-by: Daniel Stone <***@collabora.com>
---
src/scanner.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/scanner.c b/src/scanner.c
index 084f196d..47836f9a 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -35,7 +35,6 @@
#include <string.h>
#include <errno.h>
#include <ctype.h>
-#include <expat.h>
#include <getopt.h>
#include <limits.h>
#include <unistd.h>
@@ -48,6 +47,8 @@ extern char DTD_DATA_begin;
extern int DTD_DATA_len;
#endif

+#include <expat.h>
+
#include "wayland-util.h"

#define PROGRAM_NAME "wayland-scanner"

[Daniel Stone]

Clang will rightly point out that example_sockaddr_un in socket-test
will get discarded from the compilation unit as it is completely unused.
Put in a couple of lines which of no value other than stopping Clang
from complaining.

Signed-off-by: Daniel Stone <***@collabora.com>
---
tests/socket-test.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/socket-test.c b/tests/socket-test.c
index e9705628..8d39edce 100644
--- a/tests/socket-test.c
+++ b/tests/socket-test.c
@@ -42,7 +42,7 @@
* See `man 7 unix`.
*/

-static const struct sockaddr_un example_sockaddr_un;
+static struct sockaddr_un example_sockaddr_un;

#define TOO_LONG (1 + sizeof example_sockaddr_un.sun_path)

@@ -69,6 +69,11 @@ TEST(socket_path_overflow_client_connect)
d = wl_display_connect(path);
assert(d == NULL);
assert(errno == ENAMETOOLONG);
+
+ /* This is useless, but prevents a warning about example_sockaddr_un
+ * being discarded from the compilation unit. */
+ strcpy(example_sockaddr_un.sun_path, "happy now clang?");
+ assert(example_sockaddr_un.sun_path[0] != '\0');
}

TEST(socket_path_overflow_server_create)

[Emil Velikov]

Hi Dan,

Post by Daniel Stone
Clang will rightly point out that example_sockaddr_un in socket-test
will get discarded from the compilation unit as it is completely unused.
Put in a couple of lines which of no value other than stopping Clang
from complaining.
---
tests/socket-test.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/tests/socket-test.c b/tests/socket-test.c
index e9705628..8d39edce 100644
--- a/tests/socket-test.c
+++ b/tests/socket-test.c
@@ -42,7 +42,7 @@
* See `man 7 unix`.
*/
-static const struct sockaddr_un example_sockaddr_un;
+static struct sockaddr_un example_sockaddr_un;
#define TOO_LONG (1 + sizeof example_sockaddr_un.sun_path)
@@ -69,6 +69,11 @@ TEST(socket_path_overflow_client_connect)
d = wl_display_connect(path);
assert(d == NULL);
assert(errno == ENAMETOOLONG);
+
+ /* This is useless, but prevents a warning about example_sockaddr_un
+ * being discarded from the compilation unit. */
+ strcpy(example_sockaddr_un.sun_path, "happy now clang?");
+ assert(example_sockaddr_un.sun_path[0] != '\0');

Why don't you add _attrubute__((used)) instead of doing the blame
game? We already use it in the repo.

Side note: the manpage says "sun_path[108]" and also points out that
"some implementations have sun_path as short as 92 bytes"
The check in wl_socket_init_for_display_name uses sizeof, yet prints a
"exceeds 108 bytes" message.

I guess the message should be fixed?

HTH
Emil

[Daniel Stone]

Hi Emil,

Post by Emil Velikov

Post by Daniel Stone
-static const struct sockaddr_un example_sockaddr_un;
+static struct sockaddr_un example_sockaddr_un;
#define TOO_LONG (1 + sizeof example_sockaddr_un.sun_path)
@@ -69,6 +69,11 @@ TEST(socket_path_overflow_client_connect)
d = wl_display_connect(path);
assert(d == NULL);
assert(errno == ENAMETOOLONG);
+
+ /* This is useless, but prevents a warning about example_sockaddr_un
+ * being discarded from the compilation unit. */
+ strcpy(example_sockaddr_un.sun_path, "happy now clang?");
+ assert(example_sockaddr_un.sun_path[0] != '\0');

Why don't you add _attrubute__((used)) instead of doing the blame
game? We already use it in the repo.

This was already merged, but sure, a follow-up patch to do this would be fine.

Post by Emil Velikov
Side note: the manpage says "sun_path[108]" and also points out that
"some implementations have sun_path as short as 92 bytes"
The check in wl_socket_init_for_display_name uses sizeof, yet prints a
"exceeds 108 bytes" message.
I guess the message should be fixed?

Yes, it looks like a follow-up patch to use sizeof would be correct as well.

Cheers,
Daniel

[Daniel Stone]

There are far better ways to detect memory leaks, such as either
valgrind or ASan. Having Meson makes it really easy to use these tools
in our tests, and we can do that in CI as well.

Having these local wrappers actually completely broke ASan usage, so
remove them in favour of using the more powerful options.

Signed-off-by: Daniel Stone <***@collabora.com>
---
tests/sanity-test.c | 66 ++--------------------------------
tests/test-compositor.c | 5 ++-
tests/test-runner.c | 79 ++++++-----------------------------------
tests/test-runner.h | 13 +++----
4 files changed, 20 insertions(+), 143 deletions(-)

diff --git a/tests/sanity-test.c b/tests/sanity-test.c
index 2495a115..98beca8d 100644
--- a/tests/sanity-test.c
+++ b/tests/sanity-test.c
@@ -35,7 +35,7 @@

#include "test-compositor.h"

-extern int leak_check_enabled;
+extern int fd_leak_check_enabled;

TEST(empty)
{
@@ -83,71 +83,11 @@ FAIL_TEST(sanity_assert)
assert(0);
}

-FAIL_TEST(sanity_malloc_direct)
-{
- void *p;
-
- assert(leak_check_enabled);
-
- p = malloc(10); /* memory leak */
- assert(p); /* assert that we got memory, also prevents
- * the malloc from getting optimized away. */
- free(NULL); /* NULL must not be counted */
- test_disable_coredumps();
-}
-
-TEST(disable_leak_checks)
-{
- volatile void *mem;
- assert(leak_check_enabled);
- /* normally this should be on the beginning of the test.
- * Here we need to be sure, that the leak checks are
- * turned on */
- DISABLE_LEAK_CHECKS;
-
- mem = malloc(16);
- assert(mem);
-}
-
-FAIL_TEST(sanity_malloc_indirect)
-{
- struct wl_array array;
-
- assert(leak_check_enabled);
-
- wl_array_init(&array);
-
- /* call into library that calls malloc */
- wl_array_add(&array, 14);
-
- /* not freeing array, must leak */
-
- test_disable_coredumps();
-}
-
-FAIL_TEST(tc_client_memory_leaks)
-{
- struct display *d = display_create();
- client_create_noarg(d, sanity_malloc_direct);
- display_run(d);
- test_disable_coredumps();
- display_destroy(d);
-}
-
-FAIL_TEST(tc_client_memory_leaks2)
-{
- struct display *d = display_create();
- client_create_noarg(d, sanity_malloc_indirect);
- display_run(d);
- test_disable_coredumps();
- display_destroy(d);
-}
-
FAIL_TEST(sanity_fd_leak)
{
int fd[2];

- assert(leak_check_enabled);
+ assert(fd_leak_check_enabled);

/* leak 2 file descriptors */
if (pipe(fd) < 0)
@@ -185,7 +125,7 @@ sanity_fd_no_leak(void)
{
int fd[2];

- assert(leak_check_enabled);
+ assert(fd_leak_check_enabled);

/* leak 2 file descriptors */
if (pipe(fd) < 0)
diff --git a/tests/test-compositor.c b/tests/test-compositor.c
index 0631f614..72f63515 100644
--- a/tests/test-compositor.c
+++ b/tests/test-compositor.c
@@ -156,7 +156,7 @@ run_client(void (*client_main)(void *data), void *data,
int wayland_sock, int client_pipe)
{
char s[8];
- int cur_alloc, cur_fds;
+ int cur_fds;
int can_continue = 0;

/* Wait until display signals that client can continue */
@@ -169,7 +169,6 @@ run_client(void (*client_main)(void *data), void *data,
snprintf(s, sizeof s, "%d", wayland_sock);
setenv("WAYLAND_SOCKET", s, 0);

- cur_alloc = get_current_alloc_num();
cur_fds = count_open_fds();

client_main(data);
@@ -182,7 +181,7 @@ run_client(void (*client_main)(void *data), void *data,
if (!getenv("WAYLAND_SOCKET"))
cur_fds--;

- check_leaks(cur_alloc, cur_fds);
+ check_fd_leaks(cur_fds);
}

static struct client_info *
diff --git a/tests/test-runner.c b/tests/test-runner.c
index 82a0a7b8..1487dc48 100644
--- a/tests/test-runner.c
+++ b/tests/test-runner.c
@@ -44,16 +44,10 @@

#include "test-runner.h"

-static int num_alloc;
-static void* (*sys_malloc)(size_t);
-static void (*sys_free)(void*);
-static void* (*sys_realloc)(void*, size_t);
-static void* (*sys_calloc)(size_t, size_t);
-
-/* when set to 1, check if tests are not leaking memory and opened files.
+/* when set to 1, check if tests are not leaking opened files.
* It is turned on by default. It can be turned off by
* WAYLAND_TEST_NO_LEAK_CHECK environment variable. */
-int leak_check_enabled;
+int fd_leak_check_enabled;

/* when this var is set to 0, every call to test_set_timeout() is
* suppressed - handy when debugging the test. Can be set by
@@ -65,40 +59,6 @@ static int is_atty = 0;

extern const struct test __start_test_section, __stop_test_section;

-__attribute__ ((visibility("default"))) void *
-malloc(size_t size)
-{
- num_alloc++;
- return sys_malloc(size);
-}
-
-__attribute__ ((visibility("default"))) void
-free(void* mem)
-{
- if (mem != NULL)
- num_alloc--;
- sys_free(mem);
-}
-
-__attribute__ ((visibility("default"))) void *
-realloc(void* mem, size_t size)
-{
- if (mem == NULL)
- num_alloc++;
- return sys_realloc(mem, size);
-}
-
-__attribute__ ((visibility("default"))) void *
-calloc(size_t nmemb, size_t size)
-{
- if (sys_calloc == NULL)
- return NULL;
-
- num_alloc++;
-
- return sys_calloc(nmemb, size);
-}
-
static const struct test *
find_test(const char *name)
{
@@ -156,25 +116,12 @@ sigalrm_handler(int signum)
abort();
}

-int
-get_current_alloc_num(void)
-{
- return num_alloc;
-}
-
void
-check_leaks(int supposed_alloc, int supposed_fds)
+check_fd_leaks(int supposed_fds)
{
int num_fds;

- if (leak_check_enabled) {
- if (supposed_alloc != num_alloc) {
- fprintf(stderr, "Memory leak detected in test. "
- "Allocated %d blocks, unfreed %d\n", num_alloc,
- num_alloc - supposed_alloc);
- abort();
- }
-
+ if (fd_leak_check_enabled) {
num_fds = count_open_fds();
if (supposed_fds != num_fds) {
fprintf(stderr, "fd leak detected in test. "
@@ -183,14 +130,14 @@ check_leaks(int supposed_alloc, int supposed_fds)
abort();
}
} else {
- fprintf(stderr, "Leak checks disabled\n");
+ fprintf(stderr, "FD leak checks disabled\n");
}
}

static void
run_test(const struct test *t)
{
- int cur_alloc, cur_fds;
+ int cur_fds;
struct sigaction sa;

if (timeouts_enabled) {
@@ -200,7 +147,7 @@ run_test(const struct test *t)
assert(sigaction(SIGALRM, &sa, NULL) == 0);
}

- cur_alloc = get_current_alloc_num();
+ //cur_alloc = get_current_alloc_num();
cur_fds = count_open_fds();

t->run();
@@ -209,7 +156,7 @@ run_test(const struct test *t)
if (timeouts_enabled)
alarm(0);

- check_leaks(cur_alloc, cur_fds);
+ check_fd_leaks(cur_fds);

exit(EXIT_SUCCESS);
}
@@ -348,20 +295,14 @@ int main(int argc, char *argv[])
int total, pass;
siginfo_t info;

- /* Load system malloc, free, and realloc */
- sys_calloc = dlsym(RTLD_NEXT, "calloc");
- sys_realloc = dlsym(RTLD_NEXT, "realloc");
- sys_malloc = dlsym(RTLD_NEXT, "malloc");
- sys_free = dlsym(RTLD_NEXT, "free");
-
if (isatty(fileno(stderr)))
is_atty = 1;

if (is_debugger_attached()) {
- leak_check_enabled = 0;
+ fd_leak_check_enabled = 0;
timeouts_enabled = 0;
} else {
- leak_check_enabled = !getenv("WAYLAND_TEST_NO_LEAK_CHECK");
+ fd_leak_check_enabled = !getenv("WAYLAND_TEST_NO_LEAK_CHECK");
timeouts_enabled = !getenv("WAYLAND_TEST_NO_TIMEOUTS");
}

diff --git a/tests/test-runner.h b/tests/test-runner.h
index 9c47a2b0..d0734009 100644
--- a/tests/test-runner.h
+++ b/tests/test-runner.h
@@ -63,11 +63,8 @@ count_open_fds(void);
void
exec_fd_leak_check(int nr_expected_fds); /* never returns */

-int
-get_current_alloc_num(void);
-
void
-check_leaks(int supposed_allocs, int supposed_fds);
+check_fd_leaks(int supposed_fds);

/*
* set/reset the timeout in seconds. The timeout starts
@@ -89,10 +86,10 @@ test_sleep(unsigned int);
void
test_disable_coredumps(void);

-#define DISABLE_LEAK_CHECKS \
- do { \
- extern int leak_check_enabled; \
- leak_check_enabled = 0; \
+#define DISABLE_LEAK_CHECKS \
+ do { \
+ extern int fd_leak_check_enabled; \
+ fd_leak_check_enabled = 0; \
} while (0);

#endif

[Ray, Ian (GE Healthcare)]

Post by Daniel Stone
There are far better ways to detect memory leaks, such as either
valgrind or ASan. Having Meson makes it really easy to use these tools
in our tests, and we can do that in CI as well.
Having these local wrappers actually completely broke ASan usage, so
remove them in favour of using the more powerful options.
---
tests/sanity-test.c | 66 ++--------------------------------
tests/test-compositor.c | 5 ++-
tests/test-runner.c | 79 ++++++-----------------------------------
tests/test-runner.h | 13 +++----
4 files changed, 20 insertions(+), 143 deletions(-)
diff --git a/tests/sanity-test.c b/tests/sanity-test.c
index 2495a115..98beca8d 100644
--- a/tests/sanity-test.c
+++ b/tests/sanity-test.c
@@ -35,7 +35,7 @@
#include "test-compositor.h"
-extern int leak_check_enabled;
+extern int fd_leak_check_enabled;
TEST(empty)
{
@@ -83,71 +83,11 @@ FAIL_TEST(sanity_assert)
assert(0);
}
-FAIL_TEST(sanity_malloc_direct)
-{
- void *p;
-
- assert(leak_check_enabled);
-
- p = malloc(10); /* memory leak */
- assert(p); /* assert that we got memory, also prevents
- * the malloc from getting optimized away. */
- free(NULL); /* NULL must not be counted */
- test_disable_coredumps();
-}
-
-TEST(disable_leak_checks)
-{
- volatile void *mem;
- assert(leak_check_enabled);
- /* normally this should be on the beginning of the test.
- * Here we need to be sure, that the leak checks are
- * turned on */
- DISABLE_LEAK_CHECKS;
-
- mem = malloc(16);
- assert(mem);
-}
-
-FAIL_TEST(sanity_malloc_indirect)
-{
- struct wl_array array;
-
- assert(leak_check_enabled);
-
- wl_array_init(&array);
-
- /* call into library that calls malloc */
- wl_array_add(&array, 14);
-
- /* not freeing array, must leak */
-
- test_disable_coredumps();
-}
-
-FAIL_TEST(tc_client_memory_leaks)
-{
- struct display *d = display_create();
- client_create_noarg(d, sanity_malloc_direct);
- display_run(d);
- test_disable_coredumps();
- display_destroy(d);
-}
-
-FAIL_TEST(tc_client_memory_leaks2)
-{
- struct display *d = display_create();
- client_create_noarg(d, sanity_malloc_indirect);
- display_run(d);
- test_disable_coredumps();
- display_destroy(d);
-}
-
FAIL_TEST(sanity_fd_leak)
{
int fd[2];
- assert(leak_check_enabled);
+ assert(fd_leak_check_enabled);
/* leak 2 file descriptors */
if (pipe(fd) < 0)
@@ -185,7 +125,7 @@ sanity_fd_no_leak(void)
{
int fd[2];
- assert(leak_check_enabled);
+ assert(fd_leak_check_enabled);
/* leak 2 file descriptors */
if (pipe(fd) < 0)
diff --git a/tests/test-compositor.c b/tests/test-compositor.c
index 0631f614..72f63515 100644
--- a/tests/test-compositor.c
+++ b/tests/test-compositor.c
@@ -156,7 +156,7 @@ run_client(void (*client_main)(void *data), void *data,
int wayland_sock, int client_pipe)
{
char s[8];
- int cur_alloc, cur_fds;
+ int cur_fds;
int can_continue = 0;
/* Wait until display signals that client can continue */
@@ -169,7 +169,6 @@ run_client(void (*client_main)(void *data), void *data,
snprintf(s, sizeof s, "%d", wayland_sock);
setenv("WAYLAND_SOCKET", s, 0);
- cur_alloc = get_current_alloc_num();
cur_fds = count_open_fds();
client_main(data);
@@ -182,7 +181,7 @@ run_client(void (*client_main)(void *data), void *data,
if (!getenv("WAYLAND_SOCKET"))
cur_fds--;
- check_leaks(cur_alloc, cur_fds);
+ check_fd_leaks(cur_fds);
}
static struct client_info *
diff --git a/tests/test-runner.c b/tests/test-runner.c
index 82a0a7b8..1487dc48 100644
--- a/tests/test-runner.c
+++ b/tests/test-runner.c
@@ -44,16 +44,10 @@
#include "test-runner.h"
-static int num_alloc;
-static void* (*sys_malloc)(size_t);
-static void (*sys_free)(void*);
-static void* (*sys_realloc)(void*, size_t);
-static void* (*sys_calloc)(size_t, size_t);
-
-/* when set to 1, check if tests are not leaking memory and opened files.
+/* when set to 1, check if tests are not leaking opened files.
* It is turned on by default. It can be turned off by
* WAYLAND_TEST_NO_LEAK_CHECK environment variable. */
-int leak_check_enabled;
+int fd_leak_check_enabled;
/* when this var is set to 0, every call to test_set_timeout() is
* suppressed - handy when debugging the test. Can be set by
@@ -65,40 +59,6 @@ static int is_atty = 0;
extern const struct test __start_test_section, __stop_test_section;
-__attribute__ ((visibility("default"))) void *
-malloc(size_t size)
-{
- num_alloc++;
- return sys_malloc(size);
-}
-
-__attribute__ ((visibility("default"))) void
-free(void* mem)
-{
- if (mem != NULL)
- num_alloc--;
- sys_free(mem);
-}
-
-__attribute__ ((visibility("default"))) void *
-realloc(void* mem, size_t size)
-{
- if (mem == NULL)
- num_alloc++;
- return sys_realloc(mem, size);
-}
-
-__attribute__ ((visibility("default"))) void *
-calloc(size_t nmemb, size_t size)
-{
- if (sys_calloc == NULL)
- return NULL;
-
- num_alloc++;
-
- return sys_calloc(nmemb, size);
-}
-
static const struct test *
find_test(const char *name)
{
@@ -156,25 +116,12 @@ sigalrm_handler(int signum)
abort();
}
-int
-get_current_alloc_num(void)
-{
- return num_alloc;
-}
-
void
-check_leaks(int supposed_alloc, int supposed_fds)
+check_fd_leaks(int supposed_fds)
{
int num_fds;
- if (leak_check_enabled) {
- if (supposed_alloc != num_alloc) {
- fprintf(stderr, "Memory leak detected in test. "
- "Allocated %d blocks, unfreed %d\n", num_alloc,
- num_alloc - supposed_alloc);
- abort();
- }
-
+ if (fd_leak_check_enabled) {
num_fds = count_open_fds();
if (supposed_fds != num_fds) {
fprintf(stderr, "fd leak detected in test. "
@@ -183,14 +130,14 @@ check_leaks(int supposed_alloc, int supposed_fds)
abort();
}
} else {
- fprintf(stderr, "Leak checks disabled\n");
+ fprintf(stderr, "FD leak checks disabled\n");
}
}
static void
run_test(const struct test *t)
{
- int cur_alloc, cur_fds;
+ int cur_fds;
struct sigaction sa;
if (timeouts_enabled) {
@@ -200,7 +147,7 @@ run_test(const struct test *t)
assert(sigaction(SIGALRM, &sa, NULL) == 0);
}
- cur_alloc = get_current_alloc_num();
+ //cur_alloc = get_current_alloc_num();

Nit: the above line could/should have been deleted.

Post by Daniel Stone
cur_fds = count_open_fds();
t->run();
@@ -209,7 +156,7 @@ run_test(const struct test *t)
if (timeouts_enabled)
alarm(0);
- check_leaks(cur_alloc, cur_fds);
+ check_fd_leaks(cur_fds);
exit(EXIT_SUCCESS);
}
@@ -348,20 +295,14 @@ int main(int argc, char *argv[])
int total, pass;
siginfo_t info;
- /* Load system malloc, free, and realloc */
- sys_calloc = dlsym(RTLD_NEXT, "calloc");
- sys_realloc = dlsym(RTLD_NEXT, "realloc");
- sys_malloc = dlsym(RTLD_NEXT, "malloc");
- sys_free = dlsym(RTLD_NEXT, "free");
-
if (isatty(fileno(stderr)))
is_atty = 1;
if (is_debugger_attached()) {
- leak_check_enabled = 0;
+ fd_leak_check_enabled = 0;
timeouts_enabled = 0;
} else {
- leak_check_enabled = !getenv("WAYLAND_TEST_NO_LEAK_CHECK");
+ fd_leak_check_enabled = !getenv("WAYLAND_TEST_NO_LEAK_CHECK");
timeouts_enabled = !getenv("WAYLAND_TEST_NO_TIMEOUTS");
}
diff --git a/tests/test-runner.h b/tests/test-runner.h
index 9c47a2b0..d0734009 100644
--- a/tests/test-runner.h
+++ b/tests/test-runner.h
@@ -63,11 +63,8 @@ count_open_fds(void);
void
exec_fd_leak_check(int nr_expected_fds); /* never returns */
-int
-get_current_alloc_num(void);
-
void
-check_leaks(int supposed_allocs, int supposed_fds);
+check_fd_leaks(int supposed_fds);
/*
* set/reset the timeout in seconds. The timeout starts
@@ -89,10 +86,10 @@ test_sleep(unsigned int);
void
test_disable_coredumps(void);
-#define DISABLE_LEAK_CHECKS \
- do { \
- extern int leak_check_enabled; \
- leak_check_enabled = 0; \
+#define DISABLE_LEAK_CHECKS \
+ do { \
+ extern int fd_leak_check_enabled; \
+ fd_leak_check_enabled = 0; \
} while (0);
#endif
--
2.17.1
_______________________________________________
wayland-devel mailing list
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[Daniel Stone]

Hi Ian,

Post by Ray, Ian (GE Healthcare)

Post by Daniel Stone
@@ -200,7 +147,7 @@ run_test(const struct test *t)
assert(sigaction(SIGALRM, &sa, NULL) == 0);
}
- cur_alloc = get_current_alloc_num();
+ //cur_alloc = get_current_alloc_num();

Nit: the above line could/should have been deleted.

Oops, yes, thanks for catching that.

Cheers,
Daniel

[Peter Hutterer]

Post by Daniel Stone
There are far better ways to detect memory leaks, such as either
valgrind or ASan. Having Meson makes it really easy to use these tools
in our tests, and we can do that in CI as well.
Having these local wrappers actually completely broke ASan usage, so
remove them in favour of using the more powerful options.

series Reviewed-by: Peter Hutterer <***@who-t.net>

Cheers,
Peter

Post by Daniel Stone
---
tests/sanity-test.c | 66 ++--------------------------------
tests/test-compositor.c | 5 ++-
tests/test-runner.c | 79 ++++++-----------------------------------
tests/test-runner.h | 13 +++----
4 files changed, 20 insertions(+), 143 deletions(-)
diff --git a/tests/sanity-test.c b/tests/sanity-test.c
index 2495a115..98beca8d 100644
--- a/tests/sanity-test.c
+++ b/tests/sanity-test.c
@@ -35,7 +35,7 @@
#include "test-compositor.h"
-extern int leak_check_enabled;
+extern int fd_leak_check_enabled;
TEST(empty)
{
@@ -83,71 +83,11 @@ FAIL_TEST(sanity_assert)
assert(0);
}
-FAIL_TEST(sanity_malloc_direct)
-{
- void *p;
-
- assert(leak_check_enabled);
-
- p = malloc(10); /* memory leak */
- assert(p); /* assert that we got memory, also prevents
- * the malloc from getting optimized away. */
- free(NULL); /* NULL must not be counted */
- test_disable_coredumps();
-}
-
-TEST(disable_leak_checks)
-{
- volatile void *mem;
- assert(leak_check_enabled);
- /* normally this should be on the beginning of the test.
- * Here we need to be sure, that the leak checks are
- * turned on */
- DISABLE_LEAK_CHECKS;
-
- mem = malloc(16);
- assert(mem);
-}
-
-FAIL_TEST(sanity_malloc_indirect)
-{
- struct wl_array array;
-
- assert(leak_check_enabled);
-
- wl_array_init(&array);
-
- /* call into library that calls malloc */
- wl_array_add(&array, 14);
-
- /* not freeing array, must leak */
-
- test_disable_coredumps();
-}
-
-FAIL_TEST(tc_client_memory_leaks)
-{
- struct display *d = display_create();
- client_create_noarg(d, sanity_malloc_direct);
- display_run(d);
- test_disable_coredumps();
- display_destroy(d);
-}
-
-FAIL_TEST(tc_client_memory_leaks2)
-{
- struct display *d = display_create();
- client_create_noarg(d, sanity_malloc_indirect);
- display_run(d);
- test_disable_coredumps();
- display_destroy(d);
-}
-
FAIL_TEST(sanity_fd_leak)
{
int fd[2];
- assert(leak_check_enabled);
+ assert(fd_leak_check_enabled);
/* leak 2 file descriptors */
if (pipe(fd) < 0)
@@ -185,7 +125,7 @@ sanity_fd_no_leak(void)
{
int fd[2];
- assert(leak_check_enabled);
+ assert(fd_leak_check_enabled);
/* leak 2 file descriptors */
if (pipe(fd) < 0)
diff --git a/tests/test-compositor.c b/tests/test-compositor.c
index 0631f614..72f63515 100644
--- a/tests/test-compositor.c
+++ b/tests/test-compositor.c
@@ -156,7 +156,7 @@ run_client(void (*client_main)(void *data), void *data,
int wayland_sock, int client_pipe)
{
char s[8];
- int cur_alloc, cur_fds;
+ int cur_fds;
int can_continue = 0;
/* Wait until display signals that client can continue */
@@ -169,7 +169,6 @@ run_client(void (*client_main)(void *data), void *data,
snprintf(s, sizeof s, "%d", wayland_sock);
setenv("WAYLAND_SOCKET", s, 0);
- cur_alloc = get_current_alloc_num();
cur_fds = count_open_fds();
client_main(data);
@@ -182,7 +181,7 @@ run_client(void (*client_main)(void *data), void *data,
if (!getenv("WAYLAND_SOCKET"))
cur_fds--;
- check_leaks(cur_alloc, cur_fds);
+ check_fd_leaks(cur_fds);
}
static struct client_info *
diff --git a/tests/test-runner.c b/tests/test-runner.c
index 82a0a7b8..1487dc48 100644
--- a/tests/test-runner.c
+++ b/tests/test-runner.c
@@ -44,16 +44,10 @@
#include "test-runner.h"
-static int num_alloc;
-static void* (*sys_malloc)(size_t);
-static void (*sys_free)(void*);
-static void* (*sys_realloc)(void*, size_t);
-static void* (*sys_calloc)(size_t, size_t);
-
-/* when set to 1, check if tests are not leaking memory and opened files.
+/* when set to 1, check if tests are not leaking opened files.
* It is turned on by default. It can be turned off by
* WAYLAND_TEST_NO_LEAK_CHECK environment variable. */
-int leak_check_enabled;
+int fd_leak_check_enabled;
/* when this var is set to 0, every call to test_set_timeout() is
* suppressed - handy when debugging the test. Can be set by
@@ -65,40 +59,6 @@ static int is_atty = 0;
extern const struct test __start_test_section, __stop_test_section;
-__attribute__ ((visibility("default"))) void *
-malloc(size_t size)
-{
- num_alloc++;
- return sys_malloc(size);
-}
-
-__attribute__ ((visibility("default"))) void
-free(void* mem)
-{
- if (mem != NULL)
- num_alloc--;
- sys_free(mem);
-}
-
-__attribute__ ((visibility("default"))) void *
-realloc(void* mem, size_t size)
-{
- if (mem == NULL)
- num_alloc++;
- return sys_realloc(mem, size);
-}
-
-__attribute__ ((visibility("default"))) void *
-calloc(size_t nmemb, size_t size)
-{
- if (sys_calloc == NULL)
- return NULL;
-
- num_alloc++;
-
- return sys_calloc(nmemb, size);
-}
-
static const struct test *
find_test(const char *name)
{
@@ -156,25 +116,12 @@ sigalrm_handler(int signum)
abort();
}
-int
-get_current_alloc_num(void)
-{
- return num_alloc;
-}
-
void
-check_leaks(int supposed_alloc, int supposed_fds)
+check_fd_leaks(int supposed_fds)
{
int num_fds;
- if (leak_check_enabled) {
- if (supposed_alloc != num_alloc) {
- fprintf(stderr, "Memory leak detected in test. "
- "Allocated %d blocks, unfreed %d\n", num_alloc,
- num_alloc - supposed_alloc);
- abort();
- }
-
+ if (fd_leak_check_enabled) {
num_fds = count_open_fds();
if (supposed_fds != num_fds) {
fprintf(stderr, "fd leak detected in test. "
@@ -183,14 +130,14 @@ check_leaks(int supposed_alloc, int supposed_fds)
abort();
}
} else {
- fprintf(stderr, "Leak checks disabled\n");
+ fprintf(stderr, "FD leak checks disabled\n");
}
}
static void
run_test(const struct test *t)
{
- int cur_alloc, cur_fds;
+ int cur_fds;
struct sigaction sa;
if (timeouts_enabled) {
@@ -200,7 +147,7 @@ run_test(const struct test *t)
assert(sigaction(SIGALRM, &sa, NULL) == 0);
}
- cur_alloc = get_current_alloc_num();
+ //cur_alloc = get_current_alloc_num();
cur_fds = count_open_fds();
t->run();
@@ -209,7 +156,7 @@ run_test(const struct test *t)
if (timeouts_enabled)
alarm(0);
- check_leaks(cur_alloc, cur_fds);
+ check_fd_leaks(cur_fds);
exit(EXIT_SUCCESS);
}
@@ -348,20 +295,14 @@ int main(int argc, char *argv[])
int total, pass;
siginfo_t info;
- /* Load system malloc, free, and realloc */
- sys_calloc = dlsym(RTLD_NEXT, "calloc");
- sys_realloc = dlsym(RTLD_NEXT, "realloc");
- sys_malloc = dlsym(RTLD_NEXT, "malloc");
- sys_free = dlsym(RTLD_NEXT, "free");
-
if (isatty(fileno(stderr)))
is_atty = 1;
if (is_debugger_attached()) {
- leak_check_enabled = 0;
+ fd_leak_check_enabled = 0;
timeouts_enabled = 0;
} else {
- leak_check_enabled = !getenv("WAYLAND_TEST_NO_LEAK_CHECK");
+ fd_leak_check_enabled = !getenv("WAYLAND_TEST_NO_LEAK_CHECK");
timeouts_enabled = !getenv("WAYLAND_TEST_NO_TIMEOUTS");
}
diff --git a/tests/test-runner.h b/tests/test-runner.h
index 9c47a2b0..d0734009 100644
--- a/tests/test-runner.h
+++ b/tests/test-runner.h
@@ -63,11 +63,8 @@ count_open_fds(void);
void
exec_fd_leak_check(int nr_expected_fds); /* never returns */
-int
-get_current_alloc_num(void);
-
void
-check_leaks(int supposed_allocs, int supposed_fds);
+check_fd_leaks(int supposed_fds);
/*
* set/reset the timeout in seconds. The timeout starts
@@ -89,10 +86,10 @@ test_sleep(unsigned int);
void
test_disable_coredumps(void);
-#define DISABLE_LEAK_CHECKS \
- do { \
- extern int leak_check_enabled; \
- leak_check_enabled = 0; \
+#define DISABLE_LEAK_CHECKS \
+ do { \
+ extern int fd_leak_check_enabled; \
+ fd_leak_check_enabled = 0; \
} while (0);
#endif
--
2.17.1
_______________________________________________
wayland-devel mailing list
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[Emil Velikov]

Post by Daniel Stone
There are far better ways to detect memory leaks, such as either
valgrind or ASan. Having Meson makes it really easy to use these tools
in our tests, and we can do that in CI as well.
Having these local wrappers actually completely broke ASan usage, so
remove them in favour of using the more powerful options.

Fwiw adding valgrind to any build-system is a 2-5 line task ;-)
Sanitizers on the other hand (esp. on shared libraries) is a pain.

-Emil

[Daniel Stone]

Hi,

Post by Emil Velikov

Post by Daniel Stone
There are far better ways to detect memory leaks, such as either
valgrind or ASan. Having Meson makes it really easy to use these tools
in our tests, and we can do that in CI as well.
Having these local wrappers actually completely broke ASan usage, so
remove them in favour of using the more powerful options.

Fwiw adding valgrind to any build-system is a 2-5 line task ;-)

Sure, I'll happily review such a patch.

Post by Emil Velikov
Sanitizers on the other hand (esp. on shared libraries) is a pain.

Yes, I spent at least a couple of hours banging my head against that
particular brick wall last weekend, and decided not to.

Cheers,
Daniel

[Pekka Paalanen]

On Wed, 29 Aug 2018 07:17:09 +0100

Post by Daniel Stone
Hi,
These first 5 patches fix some issues I found by running the test suite
under an expanded set of toolchains, including the ASan address
sanitiser and Clang's static analyser.
Patch 6 removes the leak checking from the test suite completely.
Previously due to toolchain immaturity, it was really difficult to run
the test suite with checks for memory leaks, so we rolled our own. On
the other hand, with Meson we can now just pass '-Db_sanitize=address'
to the configure process, or run 'meson test --wrapper=valgrind', to get
more powerful and useful checkers.
The existing leak checker we have breaks ASan completely, and I couldn't
figure out how to fix it. Removing it altogether seemed like a better
idea.
I've implemented this for GitLab CI, and you can see example output
here: https://gitlab.freedesktop.org/daniels/wayland/pipelines/3663
This issue tracks the work left on the CI pipeline to get everything
https://gitlab.freedesktop.org/wayland/wayland/issues/54
If anyone wants to help out, please feel free to grab that branch and
run with it; in the meantime, these seem like good fixes to have
regardless.

Hi,

I'm thrilled where this is going. This series is:
Reviewed-by: Pekka Paalanen <***@collabora.co.uk>

Patch 3 could maybe use a code comment for being so subtle.


Thanks,
pq

[Daniel Stone]

Post by Pekka Paalanen

Post by Daniel Stone
These first 5 patches fix some issues I found by running the test suite
under an expanded set of toolchains, including the ASan address
sanitiser and Clang's static analyser.
Patch 6 removes the leak checking from the test suite completely.
Previously due to toolchain immaturity, it was really difficult to run
the test suite with checks for memory leaks, so we rolled our own. On
the other hand, with Meson we can now just pass '-Db_sanitize=address'
to the configure process, or run 'meson test --wrapper=valgrind', to get
more powerful and useful checkers.
The existing leak checker we have breaks ASan completely, and I couldn't
figure out how to fix it. Removing it altogether seemed like a better
idea.
I've implemented this for GitLab CI, and you can see example output
here: https://gitlab.freedesktop.org/daniels/wayland/pipelines/3663
This issue tracks the work left on the CI pipeline to get everything
https://gitlab.freedesktop.org/wayland/wayland/issues/54
If anyone wants to help out, please feel free to grab that branch and
run with it; in the meantime, these seem like good fixes to have
regardless.

Patch 3 could maybe use a code comment for being so subtle.

Great, I've added that now and pushed the series after a run through
CI. Thanks both for the review!

Cheers,
Daniel