Re: [PATCH 0/5] building git with clang/gcc address sanitizer

[Jeff King]

On Mon, Jul 10, 2017 at 04:40:42PM +0200, Lars Schneider wrote:

> > If you want to see it in action, you can do:
> > 
> >  make SANITIZE=address
> >  ./git log -g HEAD HEAD >/dev/null
> > 
> > which finds a bug I recently fixed (but the fix isn't in master yet).
> 
> Do you think it would make sense to run these sanitizers on TravisCI
> to ensure they keep clean? If yes, should we run only "address" or all
> of them (if they run clean)?

Maybe. It's expensive and it's relatively rare that it catches anything.
I used to run valgrind (which is even more expensive) once every release
or so. This is much cheaper, but I've noticed that the Travis
environment is a lot slower than my laptop. So it might take an hour to
run there, which I think would trigger some timeouts?

I guess the best way is to try it and see. I probably wouldn't do an
ASan run for each environment, but just one Linux ASan run, due to the
CPU expense. (TBH, I think the existing gcc versus clang on both
platforms is already slight overkill. But I guess if we have CPU to
burn, more coverage is better than less).

I think "address" is the only one that runs clean right now. With some
work I think we could get "undefined" to run clean. The others, I'm not
so sure.

Some of them can actually be combined in a single build, but I'd have to
dig into the documentation to see which (I think "thread" and "address"
don't work well together, but "undefined" and "address" might). My
SANITIZE trick doesn't handle multiple entries, but it could probably be
taught to.

-Peff

Re: [PATCH 0/5] building git with clang/gcc address sanitizer

[Lars Schneider]

> On 10 Jul 2017, at 15:24, Jeff King  wrote:
> 
> I mentioned recently that I sometimes run the test suite with ASan, so
> here are a few tweaks to make this easier (most of which I've been
> carrying in my personal config.mak for a few years).
> 
> In my experience ASan does at least as well as valgrind at finding
> memory bugs, and runs way faster. With this series, running:
> 
>  make SANITIZE=address test
> 
> takes about 4.5 minutes on my machine. A normal build+test is about 1.5
> minutes on the same machine. I haven't timed a full run with --valgrind
> recently, but I know that it is much much slower. :)
> 
> If you want to see it in action, you can do:
> 
>  make SANITIZE=address
>  ./git log -g HEAD HEAD >/dev/null
> 
> which finds a bug I recently fixed (but the fix isn't in master yet).

Do you think it would make sense to run these sanitizers on TravisCI
to ensure they keep clean? If yes, should we run only "address" or all
of them (if they run clean)?

- Lars


> 
> There are other sanitizers, too. I _thought_ I had
> 
>  make SANITIZE=undefined test
> 
> running cleanly at one point, but it's definitely not clean now. Patch 5
> helps a little by disabling unaligned loads in our get_be32() macros.
> Once upon a time I had to set INTERNAL_QSORT, but it isn't necessary
> anymore since everything goes through sane_qsort(). Most of the
> remaining bugs are of a similar form, doing something like:
> 
>  memcpy(NULL, NULL, 0);
> 
> I don't know if it's worth having a sane_memcpy() there, or simply
> tweaking the code to avoid the call (almost all of them are a single
> call from apply.c:2870).
> 
> It looks like we also have a case of shifting off the left side of a
> signed int, which is undefined.
> 
> You can also try:
> 
>  make SANITIZE=thread test
> 
> but it's not clean. I poked at some of the errors, and I don't think
> there a problem in practice (though they may be worth cleaning up in the
> name of code hygiene).
> 
> There's also:
> 
>  make SANITIZE=memory test
> 
> This is clang-only. It's supposed to find uses of uninitialized memory.
> But it complains about writing out anything that zlib has touched. I
> seem to recall that valgrind had a similar problem. I'm not sure what
> zlib does to confuse these analyzers. For valgrind we were able to add a
> suppression. We could probably do the same here, but I haven't looked
> into it.
> 
>  [1/5]: test-lib: set ASAN_OPTIONS variable before we run git
>  [2/5]: test-lib: turn on ASan abort_on_error by default
>  [3/5]: Makefile: add helper for compiling with -fsanitize
>  [4/5]: Makefile: turn off -fomit-frame-pointer with sanitizers
>  [5/5]: Makefile: disable unaligned loads with UBSan
> 
> Makefile  |  8 
> t/test-lib.sh | 11 ---
> 2 files changed, 16 insertions(+), 3 deletions(-)
> 
> -Peff

Re: [PATCH 0/5] building git with clang/gcc address sanitizer

[Jeff King]

On Mon, Jul 10, 2017 at 09:24:18AM -0400, Jeff King wrote:

> You can also try:
> 
>   make SANITIZE=thread test
> 
> but it's not clean. I poked at some of the errors, and I don't think
> there a problem in practice (though they may be worth cleaning up in the
> name of code hygiene).

Here's an example that I fixed up long ago, but never quite had the
stomach to seriously propose.

If it were the last one, it might be worth applying to get a clean run
of "make test", but I think it's just one of many.

-- >8 --
Date: Mon, 8 Dec 2014 03:02:34 -0500
Subject: [PATCH] transport-helper: initialize debug flag before starting
 threads

The transport_debug() function uses a static variable to
lazily cache the boolean value of $TRANSPORT_DEBUG. If a
program uses transport-helper's bidirectional_transfer_loop
(as remote-ext and remote-fd do), then two threads may both
try to write the variable at the same time.

We can fix this by initializing the variable right before
starting the threads.

Noticed by "-fsanitize=thread". This probably isn't a
problem in practice, as both threads will write the same
value, and we are unlikely to see a torn write on an "int"
(i.e., on sane platforms a write to an int is atomic).

Signed-off-by: Jeff King 
---
 transport-helper.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 33cff38cc..76f19ddb0 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1113,17 +1113,23 @@ int transport_helper_init(struct transport *transport, 
const char *name)
 /* This should be enough to hold debugging message. */
 #define PBUFFERSIZE 8192
 
+static int transport_debug_enabled(void)
+{
+   static int debug_enabled = -1;
+
+   if (debug_enabled < 0)
+   debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
+   return debug_enabled;
+}
+
 /* Print bidirectional transfer loop debug message. */
 __attribute__((format (printf, 1, 2)))
 static void transfer_debug(const char *fmt, ...)
 {
va_list args;
char msgbuf[PBUFFERSIZE];
-   static int debug_enabled = -1;
 
-   if (debug_enabled < 0)
-   debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
-   if (!debug_enabled)
+   if (!transport_debug_enabled())
return;
 
va_start(args, fmt);
@@ -1292,6 +1298,10 @@ static int tloop_spawnwait_tasks(struct 
bidirectional_transfer_state *s)
pthread_t ptg_thread;
int err;
int ret = 0;
+
+   /* initialize static global debug flag as a side effect */
+   transport_debug_enabled();
+
err = pthread_create(_thread, NULL, udt_copy_task_routine,
>gtp);
if (err)
-- 
2.13.2.1071.gcd8104b61

[PATCH 0/5] building git with clang/gcc address sanitizer

[Jeff King]

I mentioned recently that I sometimes run the test suite with ASan, so
here are a few tweaks to make this easier (most of which I've been
carrying in my personal config.mak for a few years).

In my experience ASan does at least as well as valgrind at finding
memory bugs, and runs way faster. With this series, running:

  make SANITIZE=address test

takes about 4.5 minutes on my machine. A normal build+test is about 1.5
minutes on the same machine. I haven't timed a full run with --valgrind
recently, but I know that it is much much slower. :)

If you want to see it in action, you can do:

  make SANITIZE=address
  ./git log -g HEAD HEAD >/dev/null

which finds a bug I recently fixed (but the fix isn't in master yet).

There are other sanitizers, too. I _thought_ I had

  make SANITIZE=undefined test

running cleanly at one point, but it's definitely not clean now. Patch 5
helps a little by disabling unaligned loads in our get_be32() macros.
Once upon a time I had to set INTERNAL_QSORT, but it isn't necessary
anymore since everything goes through sane_qsort(). Most of the
remaining bugs are of a similar form, doing something like:

  memcpy(NULL, NULL, 0);

I don't know if it's worth having a sane_memcpy() there, or simply
tweaking the code to avoid the call (almost all of them are a single
call from apply.c:2870).

It looks like we also have a case of shifting off the left side of a
signed int, which is undefined.

You can also try:

  make SANITIZE=thread test

but it's not clean. I poked at some of the errors, and I don't think
there a problem in practice (though they may be worth cleaning up in the
name of code hygiene).

There's also:

  make SANITIZE=memory test

This is clang-only. It's supposed to find uses of uninitialized memory.
But it complains about writing out anything that zlib has touched. I
seem to recall that valgrind had a similar problem. I'm not sure what
zlib does to confuse these analyzers. For valgrind we were able to add a
suppression. We could probably do the same here, but I haven't looked
into it.

  [1/5]: test-lib: set ASAN_OPTIONS variable before we run git
  [2/5]: test-lib: turn on ASan abort_on_error by default
  [3/5]: Makefile: add helper for compiling with -fsanitize
  [4/5]: Makefile: turn off -fomit-frame-pointer with sanitizers
  [5/5]: Makefile: disable unaligned loads with UBSan

 Makefile  |  8 
 t/test-lib.sh | 11 ---
 2 files changed, 16 insertions(+), 3 deletions(-)

-Peff