Bug 1005658 - Don't pass stack pointers to the GL for buffers, and have GLContext try to guard against it - r=jgilbert

gfx/gl/GLContext.cpp

@@ -1807,7 +1807,38 @@ GLContext::MarkDestroyed()
     mSymbols.Zero();
 }
 
-#ifdef MOZ_ENABLE_GL_TRACKING
+#ifdef DEBUG
+/* static */ void
+GLContext::AssertNotPassingStackBufferToTheGL(const void* ptr)
+{
+  int somethingOnTheStack;
+  const void* someStackPtr = &somethingOnTheStack;
+  const int page_bits = 12;
+  intptr_t page = reinterpret_cast<uintptr_t>(ptr) >> page_bits;
+  intptr_t someStackPage = reinterpret_cast<uintptr_t>(someStackPtr) >> page_bits;
+  uintptr_t pageDistance = std::abs(page - someStackPage);
+
+  // Explanation for the "distance <= 1" check here as opposed to just
+  // an equality check.
+  //
+  // Here we assume that pages immediately adjacent to the someStackAddress page,
+  // are also stack pages. That allows to catch the case where the calling frame put
+  // a buffer on the stack, and we just crossed the page boundary. That is likely
+  // to happen, precisely, when using stack arrays. I hit that specifically
+  // with CompositorOGL::Initialize.
+  //
+  // In theory we could be unlucky and wrongly assert here. If that happens,
+  // it will only affect debug builds, and looking at stacks we'll be able to
+  // see that this assert is wrong and revert to the conservative and safe
+  // approach of only asserting when address and someStackAddress are
+  // on the same page.
+  bool isStackAddress = pageDistance <= 1;
+  MOZ_ASSERT(!isStackAddress,
+             "Please don't pass stack arrays to the GL. "
+             "Consider using HeapCopyOfStackArray. "
+             "See bug 1005658.");
+}
+
 void
 GLContext::CreatedProgram(GLContext *aOrigin, GLuint aName)
 {

gfx/gl/GLContext.h

@@ -42,10 +42,6 @@
 #include "mozilla/Scoped.h"
 #include "gfx2DGlue.h"
 
-#ifdef DEBUG
-#define MOZ_ENABLE_GL_TRACKING 1
-#endif
-
 class nsIntRegion;
 class nsIRunnable;
 class nsIThread;
@@ -604,7 +600,7 @@ private:
 #undef BEFORE_GL_CALL
 #undef AFTER_GL_CALL
 
-#ifdef MOZ_ENABLE_GL_TRACKING
+#ifdef DEBUG
 
 #ifndef MOZ_FUNCTION_NAME
 # ifdef __GNUC__
@@ -664,6 +660,8 @@ private:
         return tip;
     }
 
+    static void AssertNotPassingStackBufferToTheGL(const void* ptr);
+
 #define BEFORE_GL_CALL                              \
             do {                                    \
                 BeforeGLCall(MOZ_FUNCTION_NAME);    \
@@ -679,11 +677,14 @@ private:
                 TrackingContext()->a;               \
             } while (0)
 
+#define ASSERT_NOT_PASSING_STACK_BUFFER_TO_GL(ptr) AssertNotPassingStackBufferToTheGL(ptr)
+
 #else // ifdef DEBUG
 
 #define BEFORE_GL_CALL do { } while (0)
 #define AFTER_GL_CALL do { } while (0)
 #define TRACKING_CONTEXT(a) do {} while (0)
+#define ASSERT_NOT_PASSING_STACK_BUFFER_TO_GL(ptr) do {} while (0)
 
 #endif // ifdef DEBUG
 
@@ -823,6 +824,7 @@ public:
 
 private:
     void raw_fBufferData(GLenum target, GLsizeiptr size, const GLvoid* data, GLenum usage) {
+        ASSERT_NOT_PASSING_STACK_BUFFER_TO_GL(data);
         BEFORE_GL_CALL;
         mSymbols.fBufferData(target, size, data, usage);
         AFTER_GL_CALL;
@@ -837,12 +839,14 @@ public:
             !data &&
             Vendor() == GLVendor::NVIDIA)
         {
-            char c = 0;
-            fBufferSubData(target, size-1, 1, &c);
+            ScopedDeleteArray<char> buf(new char[1]);
+            buf[0] = 0;
+            fBufferSubData(target, size-1, 1, buf);
         }
     }
 
     void fBufferSubData(GLenum target, GLintptr offset, GLsizeiptr size, const GLvoid* data) {
+        ASSERT_NOT_PASSING_STACK_BUFFER_TO_GL(data);
         BEFORE_GL_CALL;
         mSymbols.fBufferSubData(target, offset, size, data);
         AFTER_GL_CALL;
@@ -887,12 +891,14 @@ public:
     }
 
     void fCompressedTexImage2D(GLenum target, GLint level, GLenum internalformat, GLsizei width, GLsizei height, GLint border, GLsizei imageSize, const GLvoid *pixels) {
+        ASSERT_NOT_PASSING_STACK_BUFFER_TO_GL(pixels);
         BEFORE_GL_CALL;
         mSymbols.fCompressedTexImage2D(target, level, internalformat, width, height, border, imageSize, pixels);
         AFTER_GL_CALL;
     }
 
     void fCompressedTexSubImage2D(GLenum target, GLint level, GLint xoffset, GLint yoffset, GLsizei width, GLsizei height, GLenum format, GLsizei imageSize, const GLvoid *pixels) {
+        ASSERT_NOT_PASSING_STACK_BUFFER_TO_GL(pixels);
         BEFORE_GL_CALL;
         mSymbols.fCompressedTexSubImage2D(target, level, xoffset, yoffset, width, height, format, imageSize, pixels);
         AFTER_GL_CALL;
@@ -1398,6 +1404,7 @@ public:
     }
 
     void fTextureRangeAPPLE(GLenum target, GLsizei length, GLvoid *pointer) {
+        ASSERT_NOT_PASSING_STACK_BUFFER_TO_GL(pointer);
         BEFORE_GL_CALL;
         mSymbols.fTextureRangeAPPLE(target, length, pointer);
         AFTER_GL_CALL;
@@ -1437,6 +1444,7 @@ public:
 
 private:
     void raw_fReadPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format, GLenum type, GLvoid *pixels) {
+        ASSERT_NOT_PASSING_STACK_BUFFER_TO_GL(pixels);
         BEFORE_GL_CALL;
         mSymbols.fReadPixels(x, y, width, height, format, type, pixels);
         AFTER_GL_CALL;
@@ -1538,6 +1546,7 @@ public:
 
 private:
     void raw_fTexImage2D(GLenum target, GLint level, GLint internalformat, GLsizei width, GLsizei height, GLint border, GLenum format, GLenum type, const GLvoid *pixels) {
+        ASSERT_NOT_PASSING_STACK_BUFFER_TO_GL(pixels);
         BEFORE_GL_CALL;
         mSymbols.fTexImage2D(target, level, internalformat, width, height, border, format, type, pixels);
         AFTER_GL_CALL;
@@ -1557,6 +1566,7 @@ public:
     }
 
     void fTexSubImage2D(GLenum target, GLint level, GLint xoffset, GLint yoffset, GLsizei width, GLsizei height, GLenum format, GLenum type, const GLvoid* pixels) {
+        ASSERT_NOT_PASSING_STACK_BUFFER_TO_GL(pixels);
         BEFORE_GL_CALL;
         mSymbols.fTexSubImage2D(target, level, xoffset, yoffset, width, height, format, type, pixels);
         AFTER_GL_CALL;
@@ -2510,7 +2520,7 @@ protected:
     virtual bool MakeCurrentImpl(bool aForce) = 0;
 
 public:
-#ifdef MOZ_ENABLE_GL_TRACKING
+#ifdef DEBUG
     static void StaticInit() {
         PR_NewThreadPrivateIndex(&sCurrentGLContextTLS, nullptr);
     }
@@ -2520,7 +2530,7 @@ public:
         if (IsDestroyed()) {
             return false;
         }
-#ifdef MOZ_ENABLE_GL_TRACKING
+#ifdef DEBUG
     PR_SetThreadPrivate(sCurrentGLContextTLS, this);
 
     // XXX this assertion is disabled because it's triggering on Mac;
@@ -2967,7 +2977,7 @@ public:
 
 #undef ASSERT_SYMBOL_PRESENT
 
-#ifdef MOZ_ENABLE_GL_TRACKING
+#ifdef DEBUG
     void CreatedProgram(GLContext *aOrigin, GLuint aName);
     void CreatedShader(GLContext *aOrigin, GLuint aName);
     void CreatedBuffers(GLContext *aOrigin, GLsizei aCount, GLuint *aNames);

gfx/gl/HeapCopyOfStackArray.h

@@ -0,0 +1,47 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/* vim: set ts=8 sts=4 et sw=4 tw=80: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef HEAPCOPYOFSTACKARRAY_H_
+#define HEAPCOPYOFSTACKARRAY_H_
+
+#include "mozilla/Attributes.h"
+#include "mozilla/Scoped.h"
+
+#include <string.h>
+
+namespace mozilla {
+
+// Takes a stack array and copies it into a heap buffer.
+// Useful to retain the convenience of declaring static arrays, while
+// avoiding passing stack pointers to the GL (see bug 1005658).
+
+template <typename ElemType>
+class HeapCopyOfStackArray
+{
+public:
+  template<size_t N>
+  HeapCopyOfStackArray(ElemType (&array)[N])
+    : mArrayLength(N)
+    , mArrayData(new ElemType[N])
+  {
+    memcpy(mArrayData, &array[0], N * sizeof(ElemType));
+  }
+
+  ElemType* Data() const { return mArrayData; }
+  size_t ArrayLength() const { return mArrayLength; }
+  size_t ByteLength() const { return mArrayLength * sizeof(ElemType); }
+
+private:
+  HeapCopyOfStackArray() MOZ_DELETE;
+  HeapCopyOfStackArray(const HeapCopyOfStackArray&) MOZ_DELETE;
+
+  const size_t mArrayLength;
+  ScopedDeletePtr<ElemType> const mArrayData;
+};
+
+}
+
+#endif // HEAPCOPYOFSTACKARRAY_H_

gfx/gl/SharedSurfaceGralloc.cpp

@@ -142,9 +142,8 @@ SharedSurface_Gralloc::Fence()
     // work.  glReadPixels seems to, though.
     if (gfxPrefs::GrallocFenceWithReadPixels()) {
         mGL->MakeCurrent();
-        // read a 1x1 pixel
-        unsigned char pixels[4];
-        mGL->fReadPixels(0, 0, 1, 1, LOCAL_GL_RGBA, LOCAL_GL_UNSIGNED_BYTE, &pixels[0]);
+        ScopedDeleteArray<char> buf(new char[4]);
+        mGL->fReadPixels(0, 0, 1, 1, LOCAL_GL_RGBA, LOCAL_GL_UNSIGNED_BYTE, buf);
     }
 }
 

gfx/gl/moz.build

@@ -46,6 +46,7 @@ EXPORTS += [
     'GLTextureImage.h',
     'GLTypes.h',
     'GLUploadHelpers.h',
+    'HeapCopyOfStackArray.h',
     'ScopedGLHelpers.h',
     'SharedSurface.h',
     'SharedSurfaceEGL.h',

gfx/layers/opengl/CompositorOGL.cpp

@@ -44,6 +44,7 @@
 #include "ScopedGLHelpers.h"
 #include "GLReadTexImageHelper.h"
 #include "TiledLayerBuffer.h"           // for TiledLayerComposer
+#include "HeapCopyOfStackArray.h"
 
 #if MOZ_ANDROID_OMTC
 #include "TexturePoolOGL.h"
@@ -372,7 +373,11 @@ CompositorOGL::Initialize()
     /* Then quad texcoords */
     0.0f, 0.0f, 1.0f, 0.0f, 0.0f, 1.0f, 1.0f, 1.0f,
   };
-  mGLContext->fBufferData(LOCAL_GL_ARRAY_BUFFER, sizeof(vertices), vertices, LOCAL_GL_STATIC_DRAW);
+  HeapCopyOfStackArray<GLfloat> verticesOnHeap(vertices);
+  mGLContext->fBufferData(LOCAL_GL_ARRAY_BUFFER,
+                          verticesOnHeap.ByteLength(),
+                          verticesOnHeap.Data(),
+                          LOCAL_GL_STATIC_DRAW);
   mGLContext->fBindBuffer(LOCAL_GL_ARRAY_BUFFER, 0);
 
   nsCOMPtr<nsIConsoleService>