Explorar o código

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
Ben Clayton %!s(int64=4) %!d(string=hai) anos
pai
achega
5b99b448b3

+ 1 - 1
glslang/MachineIndependent/ShaderLang.cpp

@@ -2016,7 +2016,7 @@ bool TProgram::linkStage(EShLanguage stage, EShMessages messages)
         intermediate[stage] = new TIntermediate(stage,
         intermediate[stage] = new TIntermediate(stage,
                                                 firstIntermediate->getVersion(),
                                                 firstIntermediate->getVersion(),
                                                 firstIntermediate->getProfile());
                                                 firstIntermediate->getProfile());
-
+        intermediate[stage]->setLimits(firstIntermediate->getLimits());
 
 
         // The new TIntermediate must use the same origin as the original TIntermediates.
         // The new TIntermediate must use the same origin as the original TIntermediates.
         // Otherwise linking will fail due to different coordinate systems.
         // Otherwise linking will fail due to different coordinate systems.

+ 2 - 2
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
         // "The resulting stride (implicit or explicit), when divided by 4, must be less than or equal to the
         // implementation-dependent constant gl_MaxTransformFeedbackInterleavedComponents."
         // 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:");
             error(infoSink, "xfb_stride is too large:");
             infoSink.info.prefix(EPrefixError);
             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";
         }
         }
     }
     }
 
 

+ 20 - 1
glslang/MachineIndependent/localintermediate.h

@@ -259,6 +259,23 @@ private:
     unsigned int features;
     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.
 // Set of helper functions to help parse and build the tree.
 //
 //
@@ -270,6 +287,7 @@ public:
         profile(p), version(v),
         profile(p), version(v),
 #endif
 #endif
         treeRoot(0),
         treeRoot(0),
+        resources(TBuiltInResource{}),
         numEntryPoints(0), numErrors(0), numPushConstants(0), recursive(false),
         numEntryPoints(0), numErrors(0), numPushConstants(0), recursive(false),
         invertY(false),
         invertY(false),
         useStorageBuffer(false),
         useStorageBuffer(false),
@@ -406,6 +424,7 @@ public:
     int getNumErrors() const { return numErrors; }
     int getNumErrors() const { return numErrors; }
     void addPushConstantCount() { ++numPushConstants; }
     void addPushConstantCount() { ++numPushConstants; }
     void setLimits(const TBuiltInResource& r) { resources = r; }
     void setLimits(const TBuiltInResource& r) { resources = r; }
+    const TBuiltInResource& getLimits() const { return resources; }
 
 
     bool postProcess(TIntermNode*, EShLanguage);
     bool postProcess(TIntermNode*, EShLanguage);
     void removeTree();
     void removeTree();
@@ -955,7 +974,7 @@ protected:
     SpvVersion spvVersion;
     SpvVersion spvVersion;
     TIntermNode* treeRoot;
     TIntermNode* treeRoot;
     std::set<std::string> requestedExtensions;  // cumulation of all enabled or required extensions; not connected to what subset of the shader used them
     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 numEntryPoints;
     int numErrors;
     int numErrors;
     int numPushConstants;
     int numPushConstants;