Multiple use-after-frees in ANGLE shader translator |
||||||||||||
Issue descriptionA high level description: ANGLE uses a pool allocator that behaves like a stack allocator, where allocations can be scoped and all freed at once. In particular TCompiler::compile uses a scope (TScopedPoolAllocator) to release all temporary allocations created during compilation. Unfortunately, some objects allocated during compilation, using the pool allocator, end up outliving the scope. So stale pointers point to freed memory, which cause use-after-free when those objects are referenced again, e.g. when compiling/translating another shader. The pool allocator actually keeps around and reuse the memory pages that are freed, so for the most part the stale pointers still point to malloc'ed memory, however those pages are reused by the pool allocator for further allocations, causing all sorts of issues, like objects aliasing each other and or becoming corrupted (which can cause various second order effects). Because of this reuse behavior, ASAN doesn't directly detects the use-after-free. A simple patch ( https://chromium-review.googlesource.com/#/c/353362/ ) disables the page reuse - it also ensures each separate allocation in the pool goes to the system allocator to help tracking their source. With that, reproduction is fairly trivial, a very simple test ( https://codereview.chromium.org/2077633002 ) shows the issue under ASAN: Steps: 1- patch https://chromium-review.googlesource.com/#/c/353362/ into src/third_party/angle 2- patch https://codereview.chromium.org/2077633002 into src/ 3- build gpu_unittests under asan (I used debug and no components for better symbolization) 4- run out/Default/gpu_unittests --gtest_filter='ES3ShaderTranslatorTest.CompileTwice' Observe the output: IMPORTANT DEBUGGING NOTE: batches of tests are run inside their own process. For debugging a test inside a debugger, use the --gtest_filter=<your_test_name> flag along with --single-process-tests. Using sharding settings from environment. This is shard 0/1 Using 1 parallel jobs. Note: Google Test filter = ES3ShaderTranslatorTest.CompileTwice [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from ES3ShaderTranslatorTest [ RUN ] ES3ShaderTranslatorTest.CompileTwice ================================================================= ==85159==ERROR: AddressSanitizer: heap-use-after-free on address 0x625005494948 at pc 0x000004e181f4 bp 0x7ffd855a4f00 sp 0x7ffd855a4ef8 READ of size 4 at 0x625005494948 thread T0 #0 0x4e181f3 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4e181f3) #1 0x4e173ec (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4e173ec) #2 0x4f9f872 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4f9f872) #3 0x4fa1ab6 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4fa1ab6) #4 0x4f9faea (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4f9faea) #5 0x4f9b267 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4f9b267) #6 0x4f81f7f (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4f81f7f) #7 0x4f953bb (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4f953bb) #8 0x4d344ec (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4d344ec) #9 0x4d3bf54 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4d3bf54) #10 0x37ae7f8 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x37ae7f8) #11 0x265d3b0 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x265d3b0) #12 0x160c8fa (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x160c8fa) #13 0x376969b (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x376969b) #14 0x373a112 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x373a112) #15 0x370cf0a (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x370cf0a) #16 0x370e7c3 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x370e7c3) #17 0x3710378 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x3710378) #18 0x37272b3 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x37272b3) #19 0x3773516 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x3773516) #20 0x373f299 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x373f299) #21 0x3726716 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x3726716) #22 0x1a81030 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x1a81030) #23 0x1a7e41a (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x1a7e41a) #24 0xa9d35b (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0xa9d35b) #25 0xa9d0cc (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0xa9d0cc) #26 0xa9d016 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0xa9d016) #27 0x1b7eb13 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x1b7eb13) #28 0x1b7069f (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x1b7069f) #29 0x1b7019a (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x1b7019a) #30 0xa9c3ca (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0xa9c3ca) #31 0x7f1a43ac3f44 (/lib/x86_64-linux-gnu/libc.so.6+0x21f44) 0x625005494948 is located 72 bytes inside of 8192-byte region [0x625005494900,0x625005496900) freed by thread T0 here: #0 0x5f14ab (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x5f14ab) #1 0x4e54619 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4e54619) #2 0x4d3c1a9 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4d3c1a9) #3 0x4d3c041 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4d3c041) #4 0x37ae7f8 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x37ae7f8) #5 0x265d3b0 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x265d3b0) #6 0x160c77f (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x160c77f) #7 0x376969b (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x376969b) #8 0x373a112 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x373a112) #9 0x370cf0a (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x370cf0a) #10 0x370e7c3 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x370e7c3) #11 0x3710378 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x3710378) #12 0x37272b3 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x37272b3) #13 0x3773516 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x3773516) #14 0x373f299 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x373f299) #15 0x3726716 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x3726716) #16 0x1a81030 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x1a81030) #17 0x1a7e41a (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x1a7e41a) #18 0xa9d35b (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0xa9d35b) #19 0xa9d0cc (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0xa9d0cc) #20 0xa9d016 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0xa9d016) #21 0x1b7eb13 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x1b7eb13) #22 0x1b7069f (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x1b7069f) #23 0x1b7019a (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x1b7019a) #24 0xa9c3ca (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0xa9c3ca) #25 0x7f1a43ac3f44 (/lib/x86_64-linux-gnu/libc.so.6+0x21f44) previously allocated by thread T0 here: #0 0x5f0eeb (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x5f0eeb) #1 0x4e54ec2 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4e54ec2) #2 0x4d8e37c (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4d8e37c) #3 0x4f9f313 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4f9f313) #4 0x4f9b254 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4f9b254) #5 0x4f81f7f (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4f81f7f) #6 0x4f953bb (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4f953bb) #7 0x4d344ec (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4d344ec) #8 0x4d3bf54 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4d3bf54) #9 0x37ae7f8 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x37ae7f8) #10 0x265d3b0 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x265d3b0) #11 0x160c77f (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x160c77f) #12 0x376969b (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x376969b) #13 0x373a112 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x373a112) #14 0x370cf0a (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x370cf0a) #15 0x370e7c3 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x370e7c3) #16 0x3710378 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x3710378) #17 0x37272b3 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x37272b3) #18 0x3773516 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x3773516) #19 0x373f299 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x373f299) #20 0x3726716 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x3726716) #21 0x1a81030 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x1a81030) #22 0x1a7e41a (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x1a7e41a) #23 0xa9d35b (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0xa9d35b) #24 0xa9d0cc (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0xa9d0cc) #25 0xa9d016 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0xa9d016) #26 0x1b7eb13 (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x1b7eb13) #27 0x1b7069f (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x1b7069f) #28 0x1b7019a (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x1b7019a) #29 0xa9c3ca (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0xa9c3ca) SUMMARY: AddressSanitizer: heap-use-after-free (/usr/local/google/home/piman/work/chrome/src/out_Na/Debug/gpu_unittests+0x4e181f3) Shadow bytes around the buggy address: 0x0c4a80a8a8d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c4a80a8a8e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c4a80a8a8f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c4a80a8a900: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c4a80a8a910: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa =>0x0c4a80a8a920: fd fd fd fd fd fd fd fd fd[fd]fd fd fd fd fd fd 0x0c4a80a8a930: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c4a80a8a940: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c4a80a8a950: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c4a80a8a960: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c4a80a8a970: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Heap right redzone: fb Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack partial redzone: f4 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==85159==ABORTING [0616/183210:ERROR:kill_posix.cc(82)] Unable to terminate process group 85159: No such process [1/1] ES3ShaderTranslatorTest.CompileTwice (CRASHED) 1 test crashed: ES3ShaderTranslatorTest.CompileTwice To symbolize the stack trace, you can use addr2line - I used: "cut -d ' ' -f 6 |addr2line -fCe out/Default/gpu_unittests" and copy/paste the asan-generated backtraces. That gives: * allocation: operator new[](unsigned long) ??:? TPoolAllocator::allocate(unsigned long) ./out_Na/Debug/../../third_party/angle/src/compiler/translator/PoolAlloc.cpp:272 TType::operator new(unsigned long) ./out_Na/Debug/../../third_party/angle/src/compiler/translator/Types.h:236 (discriminator 1) BuiltInFunctionEmulator::FunctionId::FunctionId(TOperator, TType const*, TType const*) ./out_Na/Debug/../../third_party/angle/src/compiler/translator/BuiltInFunctionEmulator.cpp:206 addEmulatedFunction ./out_Na/Debug/../../third_party/angle/src/compiler/translator/BuiltInFunctionEmulator.cpp:115 InitBuiltInFunctionEmulatorForGLSLWorkarounds(BuiltInFunctionEmulator*, unsigned int) ./out_Na/Debug/../../third_party/angle/src/compiler/translator/BuiltInFunctionEmulatorGLSL.cpp:34 TranslatorGLSL::initBuiltInFunctionEmulator(BuiltInFunctionEmulator*, int) ./out_Na/Debug/../../third_party/angle/src/compiler/translator/TranslatorGLSL.cpp:26 (discriminator 1) compileTreeImpl ./out_Na/Debug/../../third_party/angle/src/compiler/translator/Compiler.cpp:321 TCompiler::compile(char const* const*, unsigned long, int) ./out_Na/Debug/../../third_party/angle/src/compiler/translator/Compiler.cpp:401 ShCompile(void*, char const* const*, unsigned long, int) ./out_Na/Debug/../../third_party/angle/src/compiler/translator/ShaderLang.cpp:249 Translate ./out_Na/Debug/../../gpu/command_buffer/service/shader_translator.cc:225 TestBody ./out_Na/Debug/../../gpu/command_buffer/service/shader_translator_unittest.cc:431 (discriminator 1) * free: operator delete[](void*) ??:? pop ./out_Na/Debug/../../third_party/angle/src/compiler/translator/PoolAlloc.cpp:186 (discriminator 1) (anonymous namespace)::TScopedPoolAllocator::~TScopedPoolAllocator() ./out_Na/Debug/../../third_party/angle/src/compiler/translator/Compiler.cpp:83 TCompiler::compile(char const* const*, unsigned long, int) ./out_Na/Debug/../../third_party/angle/src/compiler/translator/Compiler.cpp:416 ShCompile(void*, char const* const*, unsigned long, int) ./out_Na/Debug/../../third_party/angle/src/compiler/translator/ShaderLang.cpp:249 Translate ./out_Na/Debug/../../gpu/command_buffer/service/shader_translator.cc:225 TestBody ./out_Na/Debug/../../gpu/command_buffer/service/shader_translator_unittest.cc:431 (discriminator 1) * use-after-free: TType::operator==(TType const&) const ./out_Na/Debug/../../third_party/angle/src/compiler/translator/Types.h:464 TType::operator!=(TType const&) const ./out_Na/Debug/../../third_party/angle/src/compiler/translator/Types.h:473 BuiltInFunctionEmulator::FunctionId::operator<(BuiltInFunctionEmulator::FunctionId const&) const ./out_Na/Debug/../../third_party/angle/src/compiler/translator/BuiltInFunctionEmulator.cpp:235 operator() ./out_Na/Debug/../../buildtools/third_party/libc++/trunk/include/__functional_base:63 operator[] ./out_Na/Debug/../../buildtools/third_party/libc++/trunk/include/map:1555 addEmulatedFunction ./out_Na/Debug/../../third_party/angle/src/compiler/translator/BuiltInFunctionEmulator.cpp:115 (discriminator 1) InitBuiltInFunctionEmulatorForGLSLWorkarounds(BuiltInFunctionEmulator*, unsigned int) ./out_Na/Debug/../../third_party/angle/src/compiler/translator/BuiltInFunctionEmulatorGLSL.cpp:34 TranslatorGLSL::initBuiltInFunctionEmulator(BuiltInFunctionEmulator*, int) ./out_Na/Debug/../../third_party/angle/src/compiler/translator/TranslatorGLSL.cpp:26 (discriminator 1) compileTreeImpl ./out_Na/Debug/../../third_party/angle/src/compiler/translator/Compiler.cpp:321 TCompiler::compile(char const* const*, unsigned long, int) ./out_Na/Debug/../../third_party/angle/src/compiler/translator/Compiler.cpp:401 ShCompile(void*, char const* const*, unsigned long, int) ./out_Na/Debug/../../third_party/angle/src/compiler/translator/ShaderLang.cpp:249 Translate ./out_Na/Debug/../../gpu/command_buffer/service/shader_translator.cc:225 TestBody ./out_Na/Debug/../../gpu/command_buffer/service/shader_translator_unittest.cc:435 (discriminator 1) As you can see, the BuiltInFunctionEmulator::FunctionId constructor allocates a new TType into the global pool, and the memory for the TType gets freed when TCompiler::compile returns - but the FunctionId stays alive. On the second call, the stale TType pointer gets dereferenced. During my testing, I found at least another instance, where the mangled field of a valid TType gets set to a newly allocated TString inside of buildMangledName, but the memory for that TString gets similarly freed even though the TType stays around, and gets dereferenced on the second call. I'm not sure what the best way to do this is - either audit every allocation into the pool, and make sure it matches the lifetime of the owning object, or make sure to invalidate pointers when their memory gets destroyed (e.g. using a weak pointer). A third possibility is to not reuse (or free) recycled pages until all allocations into it have actually been freed - e.g. by adding a refcount (increase on allocate, decrease on deallocate).
,
Jun 17 2016
Thanks for the excellent analysis. I've put up a small CL here: https://chromium-review.googlesource.com/#/c/353671/ This should fix some (or all?) of the use-after-frees - the one I couldn't put my finger on was the mangled name re-use. Antoine can you help me identify that bug? I'd recommend we remove the TLS and global variable stack-based allocator, while keeping the pool allocator design. This would allows us to specify which pool allocates which memory explicitly. As for timing, I think we should fix all of the bugs we can find. Next quarter we could aim for replacing the global allocator and systematically solving the problem and preventing further bugs. Open to suggestions on other approaches. For reference, further discussion on removing the global allocator is in issue angleproject:1286.
,
Jun 17 2016
The following revision refers to this bug: https://chromium.googlesource.com/angle/angle/+/438dbcf15299a2abcc432d1670aa8e475ea55707 commit 438dbcf15299a2abcc432d1670aa8e475ea55707 Author: Jamie Madill <jmadill@chromium.org> Date: Fri Jun 17 18:20:05 2016 translator: Fix builtin function emulator use-after-free. We were calling the global pool allocator in the builtin function emulator, which would lead to us freeing TTypes that were still referenced. Fix this by using the TCache which was designed for such a purpose, and locking the allocator around the builtin function emulator to try and prevent similar bugs from creeping in. Eventually we would like to get rid of the global allocator and replace it with different pools in different contexts, which are managed more safely. BUG= 620937 Change-Id: If501ff6ea4d9bf8a2b8f89f2c94a01386f79ee3a Reviewed-on: https://chromium-review.googlesource.com/353671 Reviewed-by: Geoff Lang <geofflang@chromium.org> Reviewed-by: Corentin Wallez <cwallez@chromium.org> Commit-Queue: Jamie Madill <jmadill@chromium.org> [modify] https://crrev.com/438dbcf15299a2abcc432d1670aa8e475ea55707/src/compiler/translator/BuiltInFunctionEmulator.cpp [modify] https://crrev.com/438dbcf15299a2abcc432d1670aa8e475ea55707/src/compiler/translator/Compiler.cpp [modify] https://crrev.com/438dbcf15299a2abcc432d1670aa8e475ea55707/src/compiler/translator/PoolAlloc.h [modify] https://crrev.com/438dbcf15299a2abcc432d1670aa8e475ea55707/src/compiler/translator/PoolAlloc.cpp
,
Jun 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/821f13cfce94de27473bc0a95885bbcebfdde7e1 commit 821f13cfce94de27473bc0a95885bbcebfdde7e1 Author: zmo <zmo@chromium.org> Date: Mon Jun 20 21:09:59 2016 Roll ANGLE cd1b122..b098776 https://chromium.googlesource.com/angle/angle.git/+log/cd1b122..b098776 BUG= 620908 , 620937 , 616176 NOTRY=true Review-Url: https://codereview.chromium.org/2087513002 Cr-Commit-Position: refs/heads/master@{#400781} [modify] https://crrev.com/821f13cfce94de27473bc0a95885bbcebfdde7e1/DEPS
,
Jun 20 2016
Thanks for the fix. I uploaded another PS on https://codereview.chromium.org/2077633002 which exposes the buildMangledName issue - same repro steps. Note that this shader isn't really meaningful (shouldn't compile), but this was found by a fuzzer (I minimized it). Still it shouldn't cause UAFs Anyway, here are the relevant stacks: allocation: operator new[](unsigned long) ??:? TPoolAllocator::allocate(unsigned long) ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/PoolAlloc.cpp:275 pool_allocator<char>::allocate(unsigned long) ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/PoolAlloc.h:277 (discriminator 1) allocate ./outf_Ng/Debug/../../buildtools/third_party/libc++/trunk/include/memory:1488 append ./outf_Ng/Debug/../../buildtools/third_party/libc++/trunk/include/string:2582 append ./outf_Ng/Debug/../../buildtools/third_party/libc++/trunk/include/string:2687 (discriminator 2) mangledName ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/Types.h:156 (discriminator 1) buildMangledName ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/Types.cpp:162 getMangledName ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/Types.h:448 buildMangledName ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/SymbolTable.cpp:39 TFunction::getMangledName() const ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/SymbolTable.h:235 findFunction ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/ParseContext.cpp:1246 addFunctionCallOrMethod ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/ParseContext.cpp:3994 yyparse ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/glslang_tab.cpp:2485 glslang_parse(TParseContext*) ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/glslang_tab.cpp:5067 (discriminator 1) PaParseStrings(unsigned long, char const* const*, int const*, TParseContext*) ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/ParseContext.cpp:4148 compileTreeImpl ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/Compiler.cpp:233 TCompiler::compile(char const* const*, unsigned long, int) ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/Compiler.cpp:404 ShCompile(void*, char const* const*, unsigned long, int) ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/ShaderLang.cpp:249 Translate ./outf_Ng/Debug/../../gpu/command_buffer/service/shader_translator.cc:225 TestBody ./outf_Ng/Debug/../../gpu/command_buffer/service/shader_translator_unittest.cc:431 (discriminator 1) free: operator delete[](void*) ??:? pop ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/PoolAlloc.cpp:187 (discriminator 1) (anonymous namespace)::TScopedPoolAllocator::~TScopedPoolAllocator() ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/Compiler.cpp:83 TCompiler::compile(char const* const*, unsigned long, int) ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/Compiler.cpp:419 ShCompile(void*, char const* const*, unsigned long, int) ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/ShaderLang.cpp:249 Translate ./outf_Ng/Debug/../../gpu/command_buffer/service/shader_translator.cc:225 TestBody ./outf_Ng/Debug/../../gpu/command_buffer/service/shader_translator_unittest.cc:431 (discriminator 1) UAF: __asan_memcpy ??:? std::__1::char_traits<char>::copy(char*, char const*, unsigned long) ./outf_Ng/Debug/../../buildtools/third_party/libc++/trunk/include/string:652 (discriminator 2) __grow_by_and_replace ./outf_Ng/Debug/../../buildtools/third_party/libc++/trunk/include/string:2331 (discriminator 1) append ./outf_Ng/Debug/../../buildtools/third_party/libc++/trunk/include/string:2582 append ./outf_Ng/Debug/../../buildtools/third_party/libc++/trunk/include/string:2687 (discriminator 2) getMangledName ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/Types.h:448 buildMangledName ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/SymbolTable.cpp:39 TFunction::getMangledName() const ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/SymbolTable.h:235 findFunction ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/ParseContext.cpp:1246 addFunctionCallOrMethod ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/ParseContext.cpp:3994 yyparse ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/glslang_tab.cpp:2485 glslang_parse(TParseContext*) ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/glslang_tab.cpp:5067 (discriminator 1) PaParseStrings(unsigned long, char const* const*, int const*, TParseContext*) ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/ParseContext.cpp:4148 compileTreeImpl ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/Compiler.cpp:233 TCompiler::compile(char const* const*, unsigned long, int) ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/Compiler.cpp:404 ShCompile(void*, char const* const*, unsigned long, int) ./outf_Ng/Debug/../../third_party/angle/src/compiler/translator/ShaderLang.cpp:249 Translate ./outf_Ng/Debug/../../gpu/command_buffer/service/shader_translator.cc:225 TestBody ./outf_Ng/Debug/../../gpu/command_buffer/service/shader_translator_unittest.cc:435 (discriminator 1)
,
Jun 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/angle/angle/+/e064d44b9a47fe004ca3fd4f2b642448da7fce7b commit e064d44b9a47fe004ca3fd4f2b642448da7fce7b Author: Jamie Madill <jmadill@chromium.org> Date: Wed Jun 22 00:00:41 2016 translator: Fix use-after-free with DepthRange. Because this builtin uses a structure, certain shaders could trigger the mangled name to be allocated during normal shader compilation. Then when the scope is popped, the mangled name for DepthRange is freed, and we're left with a dangling pointer. Fix this temporarily by enforcing mangled name construction when we initialize the builtins, but we should look for a more robust and future-proof fix. BUG= 620937 Change-Id: If130c8b48a18054502abaec08f10264f282b4925 Reviewed-on: https://chromium-review.googlesource.com/354494 Reviewed-by: Corentin Wallez <cwallez@chromium.org> Reviewed-by: Antoine Labour <piman@google.com> Commit-Queue: Jamie Madill <jmadill@chromium.org> [modify] https://crrev.com/e064d44b9a47fe004ca3fd4f2b642448da7fce7b/src/compiler/translator/Types.h [modify] https://crrev.com/e064d44b9a47fe004ca3fd4f2b642448da7fce7b/src/compiler/translator/Initialize.cpp
,
Jun 30 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/39d447a13fdc171f98d81b87ca0a75995d5a5e78 commit 39d447a13fdc171f98d81b87ca0a75995d5a5e78 Author: geofflang <geofflang@chromium.org> Date: Thu Jun 30 16:17:17 2016 Roll ANGLE be5a8a4..3c75419 https://chromium.googlesource.com/angle/angle.git/+log/be5a8a4..3c75419 BUG= chromium:617848 , 598924 , chromium:617627 , chromium:534814 , 483282 , 620937 TBR=jmadill@chromium.org TEST=bots CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.linux:linux_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2119463002 Cr-Commit-Position: refs/heads/master@{#403192} [modify] https://crrev.com/39d447a13fdc171f98d81b87ca0a75995d5a5e78/DEPS
,
Jul 5 2016
Antoine can you re-open or make a new bug if more use-after frees come up? I'm going to mark merge-request to M53, I think the second change didn't make it in before the branch. Should we also merge to M52?
,
Jul 5 2016
,
Jul 5 2016
For the merge request: fix is in https://chromium.googlesource.com/angle/angle/+/e064d44b9a47fe004ca3fd4f2b642448da7fce7b Should be a safe change, very small, with high impact (use-after-free) and has baked in Canary for several days.
,
Jul 5 2016
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.
,
Jul 11 2016
This could be a serious issue, can we get a merge review please?
,
Jul 11 2016
,
Jul 12 2016
which branch should i merge to? 2785?
,
Jul 12 2016
Sorry, I guess I was confused by the branch points - these fixes did seem to make M53. Marking merge request for M52: For the merge request: https://crrev.com/438dbcf15299a2abcc432d1670aa8e475ea55707 https://chromium.googlesource.com/angle/angle/+/e064d44b9a47fe004ca3fd4f2b642448da7fce7b Should be safe changes, very small, with high impact (use-after-free) and have baked in Canary/M53 for several weeks.
,
Jul 12 2016
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.
,
Jul 14 2016
Approving merge to M52 branch 2743 based on comment #15. Please merge ASAP (latest by 5:00 PM PST July 15th, Friday) to make it to M52 Stable build cut.
,
Jul 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/angle/angle/+/392c3de44d214e660c753301b5453f7710328184 commit 392c3de44d214e660c753301b5453f7710328184 Author: Jamie Madill <jmadill@chromium.org> Date: Fri Jun 17 18:20:05 2016 translator: Fix builtin function emulator use-after-free. We were calling the global pool allocator in the builtin function emulator, which would lead to us freeing TTypes that were still referenced. Fix this by using the TCache which was designed for such a purpose, and locking the allocator around the builtin function emulator to try and prevent similar bugs from creeping in. Eventually we would like to get rid of the global allocator and replace it with different pools in different contexts, which are managed more safely. BUG= 620937 Change-Id: If501ff6ea4d9bf8a2b8f89f2c94a01386f79ee3a Reviewed-on: https://chromium-review.googlesource.com/353671 Reviewed-by: Geoff Lang <geofflang@chromium.org> Reviewed-by: Corentin Wallez <cwallez@chromium.org> Commit-Queue: Jamie Madill <jmadill@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/360249 Reviewed-by: Jamie Madill <jmadill@chromium.org> [modify] https://crrev.com/392c3de44d214e660c753301b5453f7710328184/src/compiler/translator/BuiltInFunctionEmulator.cpp [modify] https://crrev.com/392c3de44d214e660c753301b5453f7710328184/src/compiler/translator/Compiler.cpp [modify] https://crrev.com/392c3de44d214e660c753301b5453f7710328184/src/compiler/translator/PoolAlloc.h [modify] https://crrev.com/392c3de44d214e660c753301b5453f7710328184/src/compiler/translator/PoolAlloc.cpp
,
Jul 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/angle/angle/+/f07246f6a06d5cb90d4e3b16c3fb9170ea863d4a commit f07246f6a06d5cb90d4e3b16c3fb9170ea863d4a Author: Jamie Madill <jmadill@chromium.org> Date: Wed Jun 22 00:00:41 2016 translator: Fix use-after-free with DepthRange. Because this builtin uses a structure, certain shaders could trigger the mangled name to be allocated during normal shader compilation. Then when the scope is popped, the mangled name for DepthRange is freed, and we're left with a dangling pointer. Fix this temporarily by enforcing mangled name construction when we initialize the builtins, but we should look for a more robust and future-proof fix. BUG= 620937 Change-Id: If130c8b48a18054502abaec08f10264f282b4925 Reviewed-on: https://chromium-review.googlesource.com/354494 Reviewed-by: Corentin Wallez <cwallez@chromium.org> Reviewed-by: Antoine Labour <piman@google.com> Commit-Queue: Jamie Madill <jmadill@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/360480 Reviewed-by: Jamie Madill <jmadill@chromium.org> [modify] https://crrev.com/f07246f6a06d5cb90d4e3b16c3fb9170ea863d4a/src/compiler/translator/Types.h [modify] https://crrev.com/f07246f6a06d5cb90d4e3b16c3fb9170ea863d4a/src/compiler/translator/Initialize.cpp
,
Jul 14 2016
Seems like this is already merged to M52 branch 2743 based on comment #18. Is there anything pending for M52? If not, please remove "Merge-Approved-52" label. Thank you.
,
Jul 14 2016
weird, was that a monorail bug? it says -Merge-Approved-52 (but lower case) in #18.
,
Jul 14 2016
|
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by geoffl...@chromium.org
, Jun 17 2016