varnishcache / varnish-cache

Varnish Cache source code repository
https://www.varnish-cache.org
Other
3.56k stars 366 forks source link

SAN fixes on Ubuntu 22 #4062

Open stevendore opened 4 months ago

stevendore commented 4 months ago

Fix up some issues when configuring varnish with ASAN, UBSAN and workspace emulator on Ubuntu22. I tested locally, with all tests passing, on Bookworm as well since that is what CCI uses for the SAN builds.

  1. 109543e87fbfbda0f9dc1fb5397327f6000de277- Initialize the VDP CTX in Req_new(). I think just using INIT_OBJ is okay here and aligns with the rest of the structs initialized in this section.
  2. 6ea6c934ae1c69ef2b5bf867e4709b66bacd540f- Increase thread_pool_stack in e29.vtc. Without the increase the test would segfault on Ubuntu 22.
  3. 569c68a8630f38431b6394be1e738ba4391bea1b - Free value created from create_bogo_n_arg(). Let me know if there is a more desired way to structure the temporary value to free.

Ubsan error from Req_new():

In function ‘memset’,
    inlined from ‘Req_New’ at cache/cache_req.c:176:2:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:59:10: error: ‘__builtin_memset’ offset [0, 63] is out of the bounds [0, 0] [-Werror=array-bounds]
   59 |   return __builtin___memset_chk (__dest, __ch, __len,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   60 |                                  __glibc_objsize0 (__dest));
      |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~

Ubsan error from exec_file:

vtc.c: In function ‘exec_file’:
vtc.c:602:9: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
  602 |         macro_def(vltop, NULL, "tmpdir", "%s", tmpdir);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[4]: *** [Makefile:851: varnishtest-vtc.o] Error 1
stevendore commented 4 months ago

Looking further into https://github.com/varnishcache/varnish-cache/commit/109543e87fbfbda0f9dc1fb5397327f6000de277, the compilation errors only happen when using GCC. As for https://github.com/varnishcache/varnish-cache/commit/6ea6c934ae1c69ef2b5bf867e4709b66bacd540f this passes fine with clang instead of GCC as well. I should have been using clang. I can remove these commit all together from the PR.

For https://github.com/varnishcache/varnish-cache/commit/569c68a8630f38431b6394be1e738ba4391bea1b The test that triggered the leak under gcc but not clang. It looks pretty clearly like a leak technically so maybe clang is just being smarter?

Any opinions on how to move forward?

Edit: tests/u00000.vtc and tests/m00003.vtc are the tests that show the leak for the create_bogo_n_arg() leak on gcc only.

stevendore commented 3 months ago

Force pushed to use ZERO_OBJ instead of INIT_OBJ to clear req->vdc and skip e29.vtc when using sanitizers.