Bug 89387

Summary:	Double delete in lp_bld_misc.cpp
Product:	Mesa	Reporter:	Chris Vine <vine35792468>
Component:	Mesa core	Assignee:	mesa-dev
Status:	RESOLVED FIXED	QA Contact:	Jose Fonseca <jfonseca>
Severity:	normal
Priority:	medium
Version:	10.5
Hardware:	Other
OS:	All
Whiteboard:
i915 platform:		i915 features:

Description Chris Vine 2015-03-01 23:12:26 UTC

I have been asked in bug #86958 to open a separate bug about the resolution of the compilation error reported there, where mesa is compiled against llvm>=3.6.

To fix that compilation error, at line 504 of file lp_bld_misc.cpp the ShaderMemoryManager* object MM is passed to a unique_ptr object, which takes ownership of MM.  However, in the event of the call to EngineBuilder::create() at line 523 failing, at line 530 delete is called manually on MM, thus leading to a possible double delete (since the destructor of the unique_ptr object having ownership will also attempt to delete MM).

This may beg the question of what EngineBuilder::setMCJITMemoryManager(ShaderMemoryManager*) does in llvm < 3.6.  If that method also tries to delete its argument when finished with, you would get a double delete in the error case (ie where EngineBuilder::create() fails) for earlier versions of llvm also.  However, whatever the answer to that, the fix for bug 86958 is on the fact of it wrong.

Comment 1 Jose Fonseca 2015-03-04 14:11:52 UTC

Would this fix it?

diff --git a/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp b/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
index 5210acc..7387ffb 100644
--- a/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
+++ b/src/gallium/auxiliary/gallivm/lp_bld_misc.cpp
@@ -502,6 +502,7 @@ lp_build_create_jit_compiler_for_module(LLVMExecutionEngineRef *OutJIT,
 
 #if HAVE_LLVM >= 0x0306
        builder.setMCJITMemoryManager(std::unique_ptr<RTDyldMemoryManager>(MM));
+       MM = NULL;
 #else
        builder.setMCJITMemoryManager(MM);
 #endif

Comment 2 Chris Vine 2015-03-05 17:27:51 UTC

That will work fine.

This does beg the question whether the code is correct for llvm < 3.6 (it fixes llvm >= 3.6).

Comment 3 Jose Fonseca 2015-03-12 10:03:11 UTC

(In reply to Chris Vine from comment #2)
> That will work fine.

Thanks.

> This does beg the question whether the code is correct for llvm < 3.6 (it
> fixes llvm >= 3.6).

I believe so.

LLVM 3.4 and 3.3's documentation for  setMCJITMemoryManager/setJITMemoryManager state they only take onwership if builder.create() is successful.

And indeed we only delete MM when builder.create() fails.



Fixed on http://cgit.freedesktop.org/mesa/mesa/commit/?id=70dc8a9930f561d7ce6db7e58b5bc9b4d940e37b

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.