From 5b99b448b3dbc77f60a482b36f4e8fa9d0ff4a52 Mon Sep 17 00:00:00 2001
From: Ben Clayton <bclayton@google.com>
Date: Mon, 19 Oct 2020 22:21:12 +0100
Subject: [PATCH] Fix uninitialized use of TIntermediate::resource (#2424)

TIntermediate was constructed without initializing any of the `resources` fields,
and `TProgram::linkStage()` was not calling `TIntermediate::setLimits()`
after constructing new `TIntermediate`s for non-first stages.

Fields of `resources` were then read in `TIntermediate::finalCheck()`
triggering undefined behavior.

This CL makes three changes:
(1) `TIntermediate::setLimits()` is now called for non-first stages by
    copying the `firstIntermediate`'s limits. This ensures that the
    `resources` fields is initialized, fixing the bug.
(2) `TIntermediate::resources` is now wrapped in a `MustBeAssigned<>`
    helper struct, asserting in non-release builds that this field is
    always initialized before reading.
(3) `TIntermediate::resources` is now zero-initialized, so that if
    the `TIntermediate::resources` field is not set in a release build
    (and so the `assert()` will be disabled) behavior is still
    deterministic.

Fixes #2423
---
 glslang/MachineIndependent/ShaderLang.cpp      |  2 +-
 glslang/MachineIndependent/linkValidate.cpp    |  4 ++--
 glslang/MachineIndependent/localintermediate.h | 21 ++++++++++++++++++++-
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/glslang/MachineIndependent/ShaderLang.cpp b/glslang/MachineIndependent/ShaderLang.cpp
index 9d7f37b..c6030bd 100644
--- a/glslang/MachineIndependent/ShaderLang.cpp
+++ b/glslang/MachineIndependent/ShaderLang.cpp
@@ -2016,7 +2016,7 @@ bool TProgram::linkStage(EShLanguage stage, EShMessages messages)
         intermediate[stage] = new TIntermediate(stage,
                                                 firstIntermediate->getVersion(),
                                                 firstIntermediate->getProfile());
-
+        intermediate[stage]->setLimits(firstIntermediate->getLimits());
 
         // The new TIntermediate must use the same origin as the original TIntermediates.
         // Otherwise linking will fail due to different coordinate systems.
diff --git a/glslang/MachineIndependent/linkValidate.cpp b/glslang/MachineIndependent/linkValidate.cpp
index a8e031b..1796fed 100644
--- a/glslang/MachineIndependent/linkValidate.cpp
+++ b/glslang/MachineIndependent/linkValidate.cpp
@@ -736,10 +736,10 @@ void TIntermediate::finalCheck(TInfoSink& infoSink, bool keepUncalled)
 
         // "The resulting stride (implicit or explicit), when divided by 4, must be less than or equal to the
         // implementation-dependent constant gl_MaxTransformFeedbackInterleavedComponents."
-        if (xfbBuffers[b].stride > (unsigned int)(4 * resources.maxTransformFeedbackInterleavedComponents)) {
+        if (xfbBuffers[b].stride > (unsigned int)(4 * resources->maxTransformFeedbackInterleavedComponents)) {
             error(infoSink, "xfb_stride is too large:");
             infoSink.info.prefix(EPrefixError);
-            infoSink.info << "    xfb_buffer " << (unsigned int)b << ", components (1/4 stride) needed are " << xfbBuffers[b].stride/4 << ", gl_MaxTransformFeedbackInterleavedComponents is " << resources.maxTransformFeedbackInterleavedComponents << "\n";
+            infoSink.info << "    xfb_buffer " << (unsigned int)b << ", components (1/4 stride) needed are " << xfbBuffers[b].stride/4 << ", gl_MaxTransformFeedbackInterleavedComponents is " << resources->maxTransformFeedbackInterleavedComponents << "\n";
         }
     }
 
diff --git a/glslang/MachineIndependent/localintermediate.h b/glslang/MachineIndependent/localintermediate.h
index 3cdc1f1..f874701 100644
--- a/glslang/MachineIndependent/localintermediate.h
+++ b/glslang/MachineIndependent/localintermediate.h
@@ -259,6 +259,23 @@ private:
     unsigned int features;
 };
 
+// MustBeAssigned wraps a T, asserting that it has been assigned with 
+// operator =() before attempting to read with operator T() or operator ->().
+// Used to catch cases where fields are read before they have been assigned.
+template<typename T>
+class MustBeAssigned
+{
+public:
+    MustBeAssigned() = default;
+    MustBeAssigned(const T& v) : value(v) {}
+    operator const T&() const { assert(isSet); return value; }
+    const T* operator ->() const { assert(isSet); return &value; }
+    MustBeAssigned& operator = (const T& v) { value = v; isSet = true; return *this; } 
+private:
+    T value;
+    bool isSet = false;
+};
+
 //
 // Set of helper functions to help parse and build the tree.
 //
@@ -270,6 +287,7 @@ public:
         profile(p), version(v),
 #endif
         treeRoot(0),
+        resources(TBuiltInResource{}),
         numEntryPoints(0), numErrors(0), numPushConstants(0), recursive(false),
         invertY(false),
         useStorageBuffer(false),
@@ -406,6 +424,7 @@ public:
     int getNumErrors() const { return numErrors; }
     void addPushConstantCount() { ++numPushConstants; }
     void setLimits(const TBuiltInResource& r) { resources = r; }
+    const TBuiltInResource& getLimits() const { return resources; }
 
     bool postProcess(TIntermNode*, EShLanguage);
     void removeTree();
@@ -955,7 +974,7 @@ protected:
     SpvVersion spvVersion;
     TIntermNode* treeRoot;
     std::set<std::string> requestedExtensions;  // cumulation of all enabled or required extensions; not connected to what subset of the shader used them
-    TBuiltInResource resources;
+    MustBeAssigned<TBuiltInResource> resources;
     int numEntryPoints;
     int numErrors;
     int numPushConstants;
-- 
2.7.4