Leak in VerifyAllMetaLayers

[jbauman42]

If VerifyAllMetaLayers is called and removes a layer from the list, it'll memmove another loader_layer_properties struct on top of it. This will leak the app_key_paths, component_layer_names, and other variables.

I've seen this when using VkLayer_override.json to load a layer that's missing disable_environment. I get this LSAN error:

[00209.061] 68031.68033> [[[ELF module #0x0 "" BuildID=7a39ba59e26eda9d 0x232ee9922000]]]
[00209.061] 68031.68033> [[[ELF module #0x3 "libclang_rt.asan.so" BuildID=ab66ca36f62009de 0x22370472f000]]]
[00209.061] 68031.68033> [[[ELF module #0x6 "libfdio.so" BuildID=a7877bdc2f577057 0x23d67f7af000]]]
[00209.061] 68031.68033> [[[ELF module #0x7 "libvulkan.so" BuildID=6500b2578a9b562b 0x21460e8bb000]]]
[00209.061] 68031.68033> [[[ELF module #0x8 "libbackend_fuchsia_globals.so" BuildID=5c9e063597be7cd4 0x23a046663000]]]
[00209.061] 68031.68033> [[[ELF module #0x9 "libasync-default.so" BuildID=f4a59a37fecb4e52 0x20af2637e000]]]
[00209.061] 68031.68033> [[[ELF module #0xa "libsyslog.so" BuildID=6feb7f35d0564ff5 0x213064275000]]]
[00209.061] 68031.68033> [[[ELF module #0x2 "" BuildID=765911204172d5f5 0x4137e350d000]]]
[00209.061] 68031.68033> [[[ELF module #0x4 "libc++abi.so.1" BuildID=fe42466e8a4973a1 0x2098f7eed000]]]
[00209.061] 68031.68033> [[[ELF module #0x5 "libunwind.so.1" BuildID=6578b6935b19607d 0x232b5319f000]]]
[00209.061] 68031.68033> [[[ELF module #0x1 "libc.so" BuildID=9efdcff2e7de8b41 0x40237bbba000]]]
[00209.061] 68031.68033> [[[ELF module #0xb "libc++.so.2" BuildID=19734c9a8e4d6c73 0x204f8af03000]]]
[00209.061] 68031.68033> [[[ELF module #0xc "libtrace-engine.so" BuildID=2216e1aa27e7b692 0x23232efdb000]]]
[00209.061] 68031.68033> [[[ELF module #0xd "libvulkan_intel.so" BuildID=a6d3b12dadc48f446504fa964f66fc5542c1d2b2 0x21f922526000]]]
[00209.061] 68031.68033> [[[ELF module #0xe "VkLayer_khronos_validation.so" BuildID=58909fc169358ab4 0x2238cb5e7000]]]
[00209.061] 68031.68033>
[00209.061] 68031.68033> =================================================================
[00209.061] 68031.68033> �[1m�[31mERROR: LeakSanitizer: detected memory leaks
[00209.061] 68031.68033> �[1m�[0m
[00209.061] 68031.68033> �[1m�[34mDirect leak of 1 byte(s) in 1 object(s) allocated from:
[00209.062] 68031.68033> �[1m�[0m #0 0x00002237047849c0 in malloc() compiler-rt/lib/asan/asan_malloc_linux.cpp:129 <libclang_rt.asan.so>+0x559c0
[00209.062] 68031.68033> #1 0x000021460e983b05 in loader_instance_heap_alloc(const loader_instance*, size_t, VkSystemAllocationScope) ../../third_party/Vulkan-Loader/loader/loader.c:162 <libvulkan.so>+0xc8b05
[00209.062] 68031.68033> #2 0x000021460e9bf593 in loaderReadLayerJson(const loader_instance*, loader_layer_list*, cJSON*, layer_json_version, cJSON*, cJSON*, _Bool, char*) ../../third_party/Vulkan-Loader/loader/loader.c:3527 <libvulkan.so>+0x104593
[00209.062] 68031.68033> #3 0x000021460e99732a in loaderAddLayerProperties(const loader_instance*, loader_layer_list*, cJSON*, _Bool, char*) ../../third_party/Vulkan-Loader/loader/loader.c:3706 <libvulkan.so>+0xdc32a
[00209.062] 68031.68033> #4 0x000021460e998d22 in loaderScanForImplicitLayers(loader_instance*, loader_layer_list*) ../../third_party/Vulkan-Loader/loader/loader.c:4933 <libvulkan.so>+0xddd22
[00209.062] 68031.68033> #5 0x000021460e9b503f in terminator_EnumerateInstanceExtensionProperties(VkEnumerateInstanceExtensionPropertiesChain const*, const char*, uint32_t*, VkExtensionProperties*) ../../third_party/Vulkan-Loader/loader/loader.c:7846 <libvulkan.so>+0xfa03f
[00209.062] 68031.68033> #6 0x000021460e9c7d2e in vkEnumerateInstanceExtensionProperties(const char*, uint32_t*, VkExtensionProperties*) ../../third_party/Vulkan-Loader/loader/trampoline.c:171 <libvulkan.so>+0x10cd2e
[00209.062] 68031.68033> #7 0x0000232ee99688b5 in $anon::test_validation_layer(const char*, bool) ../../src/graphics/tests/vkvalidation/vkvalidation.cc:51 <>+0x468b5
[00209.062] 68031.68033> #8 0x0000232ee996b53d in ValidationLayers_KhronosValidationFromFile_Test::TestBody(ValidationLayers_KhronosValidationFromFile_Test*) ../../src/graphics/tests/vkvalidation/vkvalidation.cc:187 <>+0x4953d
[00209.062] 68031.68033> #9 0x0000232ee99b10e0 in testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::)(testing::Test), const char*) ../../third_party/googletest/src/googletest/src/gtest.cc:2667 <>+0x8f0e0
[00209.062] 68031.68033> #10 0x0000232ee99b0eab in testing::Test::Run(testing::Test*) ../../third_party/googletest/src/googletest/src/gtest.cc:2682 <>+0x8eeab
[00209.062] 68031.68033> #11 0x0000232ee99b25b0 in testing::TestInfo::Run(testing::TestInfo*) ../../third_party/googletest/src/googletest/src/gtest.cc:2861 <>+0x905b0
[00209.062] 68031.68033> #12 0x0000232ee99b489b in testing::TestSuite::Run(testing::TestSuite*) ../../third_party/googletest/src/googletest/src/gtest.cc:3015 <>+0x9289b
[00209.062] 68031.68033> #13 0x0000232ee99d043c in testing::internal::UnitTestImpl::RunAllTests(testing::internal::UnitTestImpl*) ../../third_party/googletest/src/googletest/src/gtest.cc:5855 <>+0xae43c
[00209.062] 68031.68033> #14 0x0000232ee99cf640 in testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::)(testing::internal::UnitTestImpl), const char*) ../../third_party/googletest/src/googletest/src/gtest.cc:2667 <>+0xad640
[00209.062] 68031.68033> #15 0x0000232ee99cf2f5 in testing::UnitTest::Run(testing::UnitTest*) ../../third_party/googletest/src/googletest/src/gtest.cc:5438 <>+0xad2f5
[00209.062] 68031.68033> #16.1 0x0000232ee99751d0 in RUN_ALL_TESTS() ../../third_party/googletest/src/googletest/include/gtest/gtest.h:2490 <>+0x531d0
[00209.062] 68031.68033> #16 0x0000232ee99751d0 in main(int, char**) ../../src/lib/fxl/test/run_all_unittests.cc:17 <>+0x531d0
[00209.062] 68031.68033> #17 0x000040237bc8cf89 in start_main(const start_params*) ../../zircon/third_party/ulib/musl/src/env/__libc_start_main.c:139 <libc.so>+0xd2f89
[00209.062] 68031.68033> #18 0x000040237bc8d991 in __libc_start_main(zx_handle_t, int (*)(int, char**, char**)) ../../zircon/third_party/ulib/musl/src/env/__libc_start_main.c:214 <libc.so>+0xd3991
[00209.062] 68031.68033>

[charles-lunarg]

I tried reproducing the issue but wasn't able to make the leak happen. Could you outline more info about the setup which caused the issue? Doing a run VK_LOADER_DEBUG=all and sending the output would be a really easy way to show what layers are on your system. I ask mainly so I when I make a fix for it, I can guarantee that it works and as a bonus add a test to make sure it doesn't happen in the future.

[jbauman42]

https://gist.github.com/jbauman42/120a081f6a6fcb85434c297ff6e4f44e is the VK_LOADER_DEBUG=all output. You can see the exact code that caused the problem at https://fuchsia-review.googlesource.com/c/fuchsia/+/578623. This is built for Fuchsia so you may not be able to run it directly, but it essentially just calls vkEnumerateInstanceExtensionProperties, which loads the VkLayer_override.json (given) which loads VkLayer_khronos_validation.json (at https://gist.github.com/jbauman42/e38c9d4a8ea162b1a49d008fb9813f35). We're using version 1.2.174 of the validation layers, but I haven't seen any code changes that would fix the problem.

[charles-lunarg]

Thank you for the additional info, this was enough info for me to directly reproduce it. Will make it very easy to fix now!

I will note that you are using a loader version 1.2.179 and before, because there was a bug that made layers loaded by the override layer always considered implicit even when they were definitely explicit layers, throwing spurious warnings. I was confused initially cause I thought the layer being loaded by the meta layer was missing the disable_environment, but it appears the meta layer itself is missing it. Now that I have the full context, reproduction is easy.

[charles-lunarg]

@jbauman42 Can you confirm that the changes fix the leak you are seeing? I went ahead and cleaned up the logic surrounding layer cleanup, this should hopefully make future leaks less likely.