Potential memory leak in dynamic_ot tracers

[lizan]

Description:
Prior to #7555, we didn't statically link libstdc++ in tests, so the test didn't fail though it is now fails under Ubuntu 18.04 with gperftools HEAPCHECK. Since we do statically link libstdc++ for released envoy binary, I suspect this is the issue living there for a while.

LSAN also detects the leak if we link libstdc++ statically in ASAN build.

Some error logs caught by Bazel CI:
https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/69421b65-80ea-4551-bcc7-7094f44dbe62/test/extensions/tracers/dynamic_ot/config_test/attempt_1.log
https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/69421b65-80ea-4551-bcc7-7094f44dbe62/test/extensions/tracers/dynamic_ot/dynamic_opentracing_driver_impl_test/attempt_1.log

[lizan]

cc @rnburn for OpenTracing C++ visibility.

I guess this is a bug of either libstdc++ or opentracing-cpp, the FlushSpans failure is kind of critical as it produced incomplete JSON and likely broken too in production.

There is no perfect solution at this moment, some workaround:

• use libc++ by using --config=libc++ to bazel, though this requires rebuilding dynamic_ot tracer with libc++ too, there is no ABI compatibility between libc++ and libstdc++.
• linking libstdc++ dynamically by adding --linkopt=-lstdc++ to bazel, note //test/exe:static_static_test will fail due to that.

[htuch]

@rnburn friendly ping; this issue is affecting a fair few users (including at Google). From some preliminary poking around, this is not in the Envoy integration but either Dynamic OT or the mock tracer that gets loaded.

[rnburn]

The tracer plugins follow the approach described here. They statically link the standard c++ library and use an export map so that the different libraries can coexist independently.

Statically linking libstdc++ in a binary and a DSO without an export map or linking the binary statically and a DSO dynamically isn't supported.

The mocktracer plugin, which was only built for testing, doesn't use an export map or static linking. If all of envoys tests now statically link the standard c++ library, maybe the best way to solve this is to add an additional mocktracer target that links to the existing target in opentracing-cpp but statically links libstdc++ (or libc++) and applies the export map.

[jmarantz]

Who owns this problem? I assume that's the person who integrated OpenTracing into Envoy?

[rnburn]

I can put in a change for this.

But to be clear, the problem was caused when the tests were changed to statically link the standard c++ library. When I originally integrated, the tests were dynamically linking to the standard c++ library and the mocktracer plugin was dynamically linking to the standard c++ library which is an approach that works fine (and is a sensible default for the mocktracer plugin).

[lizan]

@rnburn

The mocktracer plugin, which was only built for testing, doesn't use an export map or static linking.

It does, the toolchain config does applied to all C++ target built within Envoy workspace, so the mock tracer plugin is static linking libstdc++ in that sense.

 ldd bazel-bin/external/io_opentracing_cpp/mocktracer/libmocktracer_plugin.so
        linux-vdso.so.1 (0x00007ffedc79c000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f2a9f7a5000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f2a9f5a1000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f2a9f1b0000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f2a9fb4c000)

But to be clear, the problem was caused when the tests were changed to statically link the standard c++ library. When I originally integrated, the tests were dynamically linking to the standard c++ library and the mocktracer plugin was dynamically linking to the standard c++ library which is an approach that works fine (and is a sensible default for the mocktracer plugin).

Again the toolchain does apply to both, so now both tests AND mocktracer plugin is statically linked to the standard C++ library. (confirm it using ldd). The problem is not test specific as now we link tests in exactly same way that we link envoy-static binary, so this issue might live in envoy-static binary (built on ubuntu 18.04) even longer than it is revealed in tests.

Though I think there are high chance this is due to a libstdc++ bug, because doing same with libc++ doesn't cause this.

[rnburn]

Again the toolchain does apply to both, so now both tests AND mocktracer plugin is statically linked to the standard C++ library. (confirm it using ldd). The problem is not test specific as now we link tests in exactly same way that we link envoy-static binary, so this issue might live in envoy-static binary (built on ubuntu 18.04) even longer than it is revealed in tests.

Export maps (like what's done here https://github.com/lightstep/lightstep-tracer-cpp/blob/master/BUILD.bazel#L75 ) are a critical part of making this dynamic loading work.

So, if the linking was changed so that the test binary is linking to libstdc++ statically and the plugin DSO is linked statically without an export map, then, yes, you'll have issues.

Without the export map, you have two copies of libstdc++, but some of the symbols will get resolved to those in the test binaries and some will get resolved to those in the DSO.

The export map lets you link two versions of libstdc++ without any of the symbols mixing. See the notes on the SO post: https://stackoverflow.com/questions/47841812/how-to-support-dynamic-plugins-when-statically-linking-in-standard-libraries

[lizan]

@rnburn I just give the export map a quick try: https://github.com/envoyproxy/envoy/compare/master...lizan:dynamic_ot?expand=1

This does fix the FlushSpan test failure in the first comment, though heap profiler is still complains about the memory leak.

exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from //test/extensions/tracers/dynamic_ot:dynamic_opentracing_driver_impl_test
-----------------------------------------------------------------------------
WARNING: Perftools heap leak checker is active -- Performance may suffer
[==========] Running 3 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 3 tests from DynamicOpenTracingDriverTest
[ RUN      ] DynamicOpenTracingDriverTest.formatErrorMessage
[       OK ] DynamicOpenTracingDriverTest.formatErrorMessage (7 ms)
[ RUN      ] DynamicOpenTracingDriverTest.InitializeDriver
[       OK ] DynamicOpenTracingDriverTest.InitializeDriver (1 ms)
[ RUN      ] DynamicOpenTracingDriverTest.FlushSpans
[       OK ] DynamicOpenTracingDriverTest.FlushSpans (2 ms)
[----------] 3 tests from DynamicOpenTracingDriverTest (11 ms total)

[----------] Global test environment tear-down
[==========] 3 tests from 1 test suite ran. (11 ms total)
[  PASSED  ] 3 tests.
Have memory regions w/o callers: might report false leaks
Leak check _main_ detected leaks of 145408 bytes in 2 objects
The 1 largest leaks:
*** WARNING: Cannot convert addresses to symbols in output below.
*** Reason: Cannot find 'pprof' (is PPROF_PATH set correctly?)
*** If you cannot fix this, try running pprof directly.
Leak of 145408 bytes in 2 objects allocated from:
        @ 7f8e584eb096


If the preceding stack traces are not enough to find the leaks, try running THIS shell command:

pprof /home/lizan/.cache/bazel/_bazel_lizan/47a7888a6f37751b3be101c298a81af4/sandbox/linux-sandbox/1557/execroot/envoy/bazel-out/k8-fastbuild/bin/test/extensions/tracers/dynamic_ot/dynamic_opentracing_driver_impl_test.runfiles/envoy/test/extensions/tracers/dynamic_ot/dynamic_opentracing_driver_impl_test "/tmp/dynamic_opentracing_driver_impl_test.14._main_-end.heap" --inuse_objects --lines --heapcheck  --edgefraction=1e-10 --nodefraction=1e-10 --gv

If you are still puzzled about why the leaks a
Exiting with error code (instead of crashing) because of whole-program memory leaks

[rnburn]

Does gperftools' heap checker work for this type of usage with dlopen?

Its docs have this section

In order to catch all heap leaks, tcmalloc must be linked last into
your executable. The heap checker may mischaracterize some memory
accesses in libraries listed after it on the link line. For instance,
it may report these libraries as leaking memory when they're not.
(See the source code for more details.)

Could we also see what the output says with this line commented out so that we can symbols in the stack trace?

[lizan]

LeakSanitizer (which doesn't link gperftools) fails too, if you comment out BAZEL_LINKOPTS/BAZEL_LINKLIBS from .bazelrc to statically link libstdc++.

Let me try to comment out that line.

[lizan]

Hmm, if I comment out dlclose or add RTLD_NODELETE to dlopen, the test passes (in both case).

@rnburn do you think this is a false positive of LSAN and gperftools profiler?

[rnburn]

I think it's likely. I've found the sanitizer to have trouble working with dlopen: I ran into this other false positive with UBSAN google/sanitizers#911

[stale]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[antoniovicente]

I have noticed that in my local builds the following tests fail with a dynamic load exception. Is this the same problem? Are there known work-arounds?

Tests:
//test/extensions/tracers/dynamic_ot:config_test
//test/extensions/tracers/dynamic_ot:dynamic_opentracing_driver_impl_test

Failure:
unknown file: Failure
C++ exception with description "opentracing: failed to load dynamic library: /home/avd/.cache/bazel/_bazel_avd/1e20bf3ad6fa0170c07bc768f0392847/sandbox/linux-sandbox/5154/execroot/envoy/bazel-out/k8-fastbuild/bin/test/extensions/tracers/dynamic_ot/config_test.runfiles/io_opentracing_cpp/mocktracer/libmocktracer_plugin.so: undefined symbol: OpenTracingMakeTracerFactory" thrown in the test body.

[PiotrSikora]

@antoniovicente it's because of libc++ vs stdlibc++, see: #4251 (comment).

[antoniovicente]

Are there known work arounds to make the two tests in question succeed on local builds?

[PiotrSikora]

Oops, that would indeed be a more helpful response. Try using --config=libc++:

bazel build --config=libc++ //source/exe:envoy
bazel test  --config=libc++ //test/...

[antoniovicente]

bazel test --config=libc++ seems to trigger a different set of problems on tests like //test/common/access_log:access_log_formatter_fuzz_test at least on my machine.

/home/avd/.cache/bazel/_bazel_avd/1e20bf3ad6fa0170c07bc768f0392847/sandbox/linux-sandbox/7410/execroot/envoy/bazel-out/k8-fastbuild/bin/test/common/access_log/access_log_formatter_fuzz_test.runfiles/envoy/test/common/access_log/access_log_formatter_fuzz_test: error while loading shared libraries: libc++.so.1: cannot open shared object file: No such file or directory

[jmarantz]

I've punted this one... Those two tests fail for me always when running locally, and I just count on CI at this point.

Not a great state and I would love for a solution to exist for however our workstations are configured.

[lizan]

@antoniovicente likely you didn't install libc++ to standard location, and didn't add the path to your ldconfig

[PiotrSikora]

@lizan aren't tests linked statically anymore?

[lizan]

@PiotrSikora fuzzer targets are always dynamic linking libc++

[aishwaryabk]

@lizan Is this issue solved? I am working on testing envoy on rhel7.6 ppc64le. I am facing similar errors in
//test/extensions/tracers/dynamic_ot:config_test
//test/extensions/tracers/dynamic_ot:dynamic_opentracing_driver_impl_test

Failure:
unknown file: Failure
C++ exception with description "opentracing: failed to load dynamic library: /root/.cache/bazel/_bazel_root/37e3aec351bcd85a6ea8b58e3592ef6e/sandbox/processwrapper-sandbox/5841/execroot/envoy/bazel-out/ppc-fastbuild/bin/test/extensions/tracers/dynamic_ot/config_test.runfiles/envoy/external/io_opentracing_cpp/mocktracer/libmocktracer_plugin.so: undefined symbol: OpenTracingMakeTracerFactory" thrown in the test body.
[ FAILED ] DynamicOtTracerConfigTest.DynamicOpentracingHttpTracer

Thread finding failed with -1 errno=1
Could not find thread stacks. Will likely report false leak positives.
Have memory regions w/o callers: might report false leaks
No leaks found for check "_main_" (but no 100% guarantee that there aren't any): found 12009 reachable heap objects of 942391 bytes

Would like some help on understanding the same.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[github-actions]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.