<div dir="ltr">On Sat, Aug 17, 2013 at 10:43 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Greetings folks,<div><br></div><div>Manuel pointed me at a use-after-free issue that is really nasty. We've got a lot of these while using preambles in libclang for code completion. The crux of the problem seems to stem from the following:</div>


<div><br></div><div>- DeclContext::lookup(...) returns a MutableArrayRef<NamedDecl*> which points into a particular StoredDeclsList looked up in a StoredDeclsMap.</div><div><br></div><div>- In some cases, the StoredDeclsList only has one element, and it stores it internally.</div>


<div><br></div><div>- In other cases, the StoredDeclsList stores the elements in a small vector which can grow and be re-allocated...</div><div><br></div><div>- Thus, things which add decls to an AST have a very real chance of invalidating the pointer stored in the MutableArrayRef returned by DeclContext::lookup.</div>


<div><br></div><div>=/</div><div><br></div><div>My question is how best to fix this long term? I can go in and add targeted copying of the NamedDecl*s in the lookup result when there are clearly operations that might shift the AST... I've started looking for these and may commit a few strategic fixes that are causing crashes for us,</div>

</div></blockquote><div><br></div><div>As with any iterator invalidation problem, the burden is on the caller to not hold an iterator across an operation that might mutate the container, so that doesn't sound completely terrible. Perhaps we should add a debug mode feature to force the StoredDeclsList to re(heap-)allocate any time it's touched, so that ASan can reliably detect these?</div>
<div><br></div><div>It seems to me that the risk is with cases where Sema implicitly adds declarations (special members or maybe builtins) to a context as part of some innocent-seeming operation -- otherwise, it should be pretty obvious that you're creating a declaration while in the middle of iterating over a lookup result.</div>
<div><br></div><div>I've gone through the callers of DeclContext::lookup, and most of them look fine. The exceptions are:</div><div><br></div><div>  * SemaLookup's LookupSpecialMembers is incorrect, because AddTemplateOverloadCandidate performs template argument deduction and thus can trigger the declaration of special members.</div>
<div>  * SemaDecl's FindOverriddenMethod looks suspicious, because it's not obvious that IsOverload can't trigger an implicit declaration of a special member</div><div>  * SemaDeclCXX's FindHiddenVirtualMethod looks suspicious for the same reason</div>
<div>  * The CXXBasePath issue you note below</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>but it seems like a losing proposition because the results of a lookup are sometimes rather long lived -- we store them in CXXBasePaths for example.</div>

</div></blockquote><div><br></div><div>Yeah, that looks bad... In mitigation, we don't store CXXBasePaths long-term, and the lookup result stored there genuinely shouldn't change once we've built the object, so this is probably not a problem today. But I don't see any issue with making a copy there.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">
<div>Is there a better long-term solution? Other ideas or suggestions? Have I messed up my analysis somewhere?</div></div></blockquote><div><br></div><div>I think this is just a few point bugs rather than a widespread systemic failure, but it's certainly suboptimal. For the most part, we're immune to problems here because most uses of lookup build a LookupResult rather than navigating the DeclContext's declarations directly.</div>
</div></div></div>