[llvm][mustache] Fix UB in ASTNode::render()

[ilovepi]

The current implementation set a reference to a nullptr, leading to all
kinds of problems. Instead, we can check the various uses to ensure we
don't deref invalid memory, and improve the logic for how contexts are
passed to children, since that was also subtly wrong in some cases.

[ilovepi]

• [llvm][mustache] Fix UB in ASTNode::render() #142249 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Paul Kirth (ilovepi)

Changes

The current implementation set a reference to a nullptr, leading to all
kinds of problems. Instead, we can check the various uses to ensure we
don't deref invalid memory, and improve the logic for how contexts are
passed to children, since that was also subtly wrong in some cases.

---

Full diff: https://github.com/llvm/llvm-project/pull/142249.diff

1 Files Affected:

• (modified) llvm/lib/Support/Mustache.cpp (+33-22)

diff --git a/llvm/lib/Support/Mustache.cpp b/llvm/lib/Support/Mustache.cpp
index 89900ed4d03a5..187e90e101a13 100644
--- a/llvm/lib/Support/Mustache.cpp
+++ b/llvm/lib/Support/Mustache.cpp
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 #include "llvm/Support/Mustache.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/IR/Value.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_ostream.h"
 #include <sstream>
@@ -23,6 +24,13 @@ static bool isFalsey(const json::Value &V) {
          (V.getAsArray() && V.getAsArray()->empty());
 }
 
+static bool isContextFalsey(const json::Value *V) {
+  // A missing context (represented by a nullptr) is defined as falsey.
+  if (!V)
+    return true;
+  return isFalsey(*V);
+}
+
 static Accessor splitMustacheString(StringRef Str) {
   // We split the mustache string into an accessor.
   // For example:
@@ -560,14 +568,15 @@ void toMustacheString(const json::Value &Data, raw_ostream &OS) {
   }
 }
 
-void ASTNode::render(const json::Value &Data, raw_ostream &OS) {
-  ParentContext = &Data;
+void ASTNode::render(const json::Value &CurrentCtx, raw_ostream &OS) {
+  // Set the parent context to the incoming context so that we
+  // can walk up the context tree correctly in findContext().
+  ParentContext = &CurrentCtx;
   const json::Value *ContextPtr = Ty == Root ? ParentContext : findContext();
-  const json::Value &Context = ContextPtr ? *ContextPtr : nullptr;
 
   switch (Ty) {
   case Root:
-    renderChild(Data, OS);
+    renderChild(CurrentCtx, OS);
     return;
   case Text:
     OS << Body;
@@ -575,53 +584,55 @@ void ASTNode::render(const json::Value &Data, raw_ostream &OS) {
   case Partial: {
     auto Partial = Partials.find(AccessorValue[0]);
     if (Partial != Partials.end())
-      renderPartial(Data, OS, Partial->getValue().get());
+      renderPartial(CurrentCtx, OS, Partial->getValue().get());
     return;
   }
   case Variable: {
     auto Lambda = Lambdas.find(AccessorValue[0]);
-    if (Lambda != Lambdas.end())
-      renderLambdas(Data, OS, Lambda->getValue());
-    else {
+    if (Lambda != Lambdas.end()) {
+      renderLambdas(CurrentCtx, OS, Lambda->getValue());
+    } else if (ContextPtr) {
       EscapeStringStream ES(OS, Escapes);
-      toMustacheString(Context, ES);
+      toMustacheString(*ContextPtr, ES);
     }
     return;
   }
   case UnescapeVariable: {
     auto Lambda = Lambdas.find(AccessorValue[0]);
-    if (Lambda != Lambdas.end())
-      renderLambdas(Data, OS, Lambda->getValue());
-    else
-      toMustacheString(Context, OS);
+    if (Lambda != Lambdas.end()) {
+      renderLambdas(CurrentCtx, OS, Lambda->getValue());
+    } else if (ContextPtr) {
+      toMustacheString(*ContextPtr, OS);
+    }
     return;
   }
   case Section: {
-    // Sections are not rendered if the context is falsey.
     auto SectionLambda = SectionLambdas.find(AccessorValue[0]);
     bool IsLambda = SectionLambda != SectionLambdas.end();
-    if (isFalsey(Context) && !IsLambda)
+
+    if (isContextFalsey(ContextPtr) && !IsLambda)
       return;
 
     if (IsLambda) {
-      renderSectionLambdas(Data, OS, SectionLambda->getValue());
+      renderSectionLambdas(CurrentCtx, OS, SectionLambda->getValue());
       return;
     }
 
-    if (Context.getAsArray()) {
-      const json::Array *Arr = Context.getAsArray();
+    if (const json::Array *Arr = ContextPtr->getAsArray()) {
       for (const json::Value &V : *Arr)
         renderChild(V, OS);
       return;
     }
-    renderChild(Context, OS);
+    renderChild(*ContextPtr, OS);
     return;
   }
   case InvertSection: {
     bool IsLambda = SectionLambdas.contains(AccessorValue[0]);
-    if (!isFalsey(Context) || IsLambda)
-      return;
-    renderChild(Context, OS);
+    if (isContextFalsey(ContextPtr) && !IsLambda) {
+      // The context for the children remains UNCHANGED from the parent's.
+      // We pass 'Data', which is this node's original incoming context.
+      renderChild(CurrentCtx, OS);
+    }
     return;
   }
   }

[nikic]

Not related to your change, but seeing this, I wonder why we don't also have to reset ParentContext to the original value when leaving the recursion?

[ilovepi]

That ... is a good point. I'll need to look at this code some more. @PeterChou1 do you recall?

[ilovepi]

So, think the parent of the node at this point is default/root node. Here we're setting it to the actual parent. I don't think it needs to be reset on the way back up, since they're now fixed in the template/AST. This does make me wonder if there is a better way to set the context/parent relationships than in this recursive routine, though.

[PeterChou1]

The ASTNode is a tree of nodes with the parentContext originally set to null. As we render the moustache template we set the parent context from the incoming parent node. We don't need to reset the parentContext when leaving because its was null originally

[evelez7]

LGTM. This fixes the ASan problem I was experiencing. 🎊

[ilovepi]

Merge activity

• Jun 4, 4:45 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Jun 4, 4:46 PM UTC: @ilovepi merged this pull request with Graphite.