Bug 1005658 - Don't pass stack pointers to the GL for buffers, and have GLContext try to guard against it - r=jgilbert
authorBenoit Jacob <bjacob@mozilla.com>
Thu, 08 May 2014 21:03:37 -0400
changeset 182246 fbef254f0aa5c1eb8b6c635d1f3a1d8a41b89c1c
parent 182245 b347f6eb2239cf0c08eef7af4f6d9eb36a0eda99
child 182247 b66e279688a1c90afa9f95c9259059df3a7bfa97
push id43247
push userbjacob@mozilla.com
push dateFri, 09 May 2014 01:04:11 +0000
treeherdermozilla-inbound@fbef254f0aa5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjgilbert
bugs1005658
milestone32.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
gfx/gl/GLContext.h
gfx/gl/HeapCopyOfStackArray.h
gfx/gl/SharedSurfaceGralloc.cpp
gfx/gl/moz.build
gfx/layers/opengl/CompositorOGL.cpp
--- a/gfx/gl/GLContext.cpp
+++ b/gfx/gl/GLContext.cpp
@@ -1802,17 +1802,48 @@ GLContext::MarkDestroyed()
         mTexGarbageBin->GLContextTeardown();
     } else {
         NS_WARNING("MakeCurrent() failed during MarkDestroyed! Skipping GL object teardown.");
     }
 
     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)
 {
     mTrackedPrograms.AppendElement(NamedResource(aOrigin, aName));
 }
 
 void
 GLContext::CreatedShader(GLContext *aOrigin, GLuint aName)
--- a/gfx/gl/GLContext.h
+++ b/gfx/gl/GLContext.h
@@ -37,20 +37,16 @@
 #include "GLTextureImage.h"
 #include "SurfaceTypes.h"
 #include "GLScreenBuffer.h"
 #include "GLContextSymbols.h"
 #include "mozilla/GenericRefCounted.h"
 #include "mozilla/Scoped.h"
 #include "gfx2DGlue.h"
 
-#ifdef DEBUG
-#define MOZ_ENABLE_GL_TRACKING 1
-#endif
-
 class nsIntRegion;
 class nsIRunnable;
 class nsIThread;
 
 namespace android {
     class GraphicBuffer;
 }
 
@@ -599,17 +595,17 @@ private:
 
 // -----------------------------------------------------------------------------
 // MOZ_GL_DEBUG implementation
 private:
 
 #undef BEFORE_GL_CALL
 #undef AFTER_GL_CALL
 
-#ifdef MOZ_ENABLE_GL_TRACKING
+#ifdef DEBUG
 
 #ifndef MOZ_FUNCTION_NAME
 # ifdef __GNUC__
 #  define MOZ_FUNCTION_NAME __PRETTY_FUNCTION__
 # elif defined(_MSC_VER)
 #  define MOZ_FUNCTION_NAME __FUNCTION__
 # else
 #  define MOZ_FUNCTION_NAME __func__  // defined in C99, supported in various C++ compilers. Just raw function name.
@@ -659,36 +655,41 @@ private:
     GLContext *TrackingContext()
     {
         GLContext *tip = this;
         while (tip->mSharedContext)
             tip = tip->mSharedContext;
         return tip;
     }
 
+    static void AssertNotPassingStackBufferToTheGL(const void* ptr);
+
 #define BEFORE_GL_CALL                              \
             do {                                    \
                 BeforeGLCall(MOZ_FUNCTION_NAME);    \
             } while (0)
 
 #define AFTER_GL_CALL                               \
             do {                                    \
                 AfterGLCall(MOZ_FUNCTION_NAME);     \
             } while (0)
 
 #define TRACKING_CONTEXT(a)                         \
             do {                                    \
                 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
 
 #define ASSERT_SYMBOL_PRESENT(func) \
             do {\
                 MOZ_ASSERT(strstr(MOZ_FUNCTION_NAME, #func) != nullptr, "Mismatched symbol check.");\
                 if (MOZ_UNLIKELY(!mSymbols.func)) {\
                     printf_stderr("RUNTIME ASSERT: Uninitialized GL function: %s\n", #func);\
@@ -818,36 +819,39 @@ public:
     void fBlendFuncSeparate(GLenum sfactorRGB, GLenum dfactorRGB, GLenum sfactorAlpha, GLenum dfactorAlpha) {
         BEFORE_GL_CALL;
         mSymbols.fBlendFuncSeparate(sfactorRGB, dfactorRGB, sfactorAlpha, dfactorAlpha);
         AFTER_GL_CALL;
     }
 
 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;
     }
 
 public:
     void fBufferData(GLenum target, GLsizeiptr size, const GLvoid* data, GLenum usage) {
         raw_fBufferData(target, size, data, usage);
 
         // bug 744888
         if (WorkAroundDriverBugs() &&
             !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;
     }
 
 private:
     void raw_fClear(GLbitfield mask) {
         BEFORE_GL_CALL;
@@ -882,22 +886,24 @@ public:
 
     void fColorMask(realGLboolean red, realGLboolean green, realGLboolean blue, realGLboolean alpha) {
         BEFORE_GL_CALL;
         mSymbols.fColorMask(red, green, blue, alpha);
         AFTER_GL_CALL;
     }
 
     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;
     }
 
     void fCopyTexImage2D(GLenum target, GLint level, GLenum internalformat, GLint x, GLint y, GLsizei width, GLsizei height, GLint border) {
         if (!IsTextureSizeSafeToPassToDriver(target, width, height)) {
             // pass wrong values to cause the GL to generate GL_INVALID_VALUE.
@@ -1393,16 +1399,17 @@ public:
 
     void fPixelStorei(GLenum pname, GLint param) {
         BEFORE_GL_CALL;
         mSymbols.fPixelStorei(pname, param);
         AFTER_GL_CALL;
     }
 
     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;
     }
 
     void fPointParameterf(GLenum pname, GLfloat param) {
         BEFORE_GL_CALL;
         mSymbols.fPointParameterf(pname, param);
@@ -1432,16 +1439,17 @@ public:
     void fReadBuffer(GLenum mode) {
         BEFORE_GL_CALL;
         mSymbols.fReadBuffer(mode);
         AFTER_GL_CALL;
     }
 
 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;
     }
 
 public:
     void fReadPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format, GLenum type, GLvoid *pixels) {
         BeforeGLReadCall();
@@ -1533,16 +1541,17 @@ public:
     void fTexGenfv(GLenum coord, GLenum pname, const GLfloat *params) {
         BEFORE_GL_CALL;
         mSymbols.fTexGenfv(coord, pname, params);
         AFTER_GL_CALL;
     }
 
 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;
     }
 
 public:
     void fTexImage2D(GLenum target, GLint level, GLint internalformat, GLsizei width, GLsizei height, GLint border, GLenum format, GLenum type, const GLvoid *pixels) {
         if (!IsTextureSizeSafeToPassToDriver(target, width, height)) {
@@ -1552,16 +1561,17 @@ public:
             width = -1;
             height = -1;
             border = -1;
         }
         raw_fTexImage2D(target, level, internalformat, width, height, border, format, type, pixels);
     }
 
     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;
     }
 
     void fUniform1f(GLint location, GLfloat v0) {
         BEFORE_GL_CALL;
         mSymbols.fUniform1f(location, v0);
@@ -2505,27 +2515,27 @@ public:
 protected:
     typedef class gfx::SharedSurface SharedSurface;
     typedef gfx::SharedSurfaceType SharedSurfaceType;
     typedef gfx::SurfaceFormat SurfaceFormat;
 
     virtual bool MakeCurrentImpl(bool aForce) = 0;
 
 public:
-#ifdef MOZ_ENABLE_GL_TRACKING
+#ifdef DEBUG
     static void StaticInit() {
         PR_NewThreadPrivateIndex(&sCurrentGLContextTLS, nullptr);
     }
 #endif
 
     bool MakeCurrent(bool aForce = false) {
         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;
     // we need to figure out why and reenable it.
 #if 0
         // IsOwningThreadCurrent is a bit of a misnomer;
         // the "owning thread" is the creation thread,
         // and the only thread that can own this.  We don't
@@ -2962,17 +2972,17 @@ public:
         mViewportRect[3] = height;
         BEFORE_GL_CALL;
         mSymbols.fViewport(x, y, width, height);
         AFTER_GL_CALL;
     }
 
 #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);
     void CreatedQueries(GLContext *aOrigin, GLsizei aCount, GLuint *aNames);
     void CreatedTextures(GLContext *aOrigin, GLsizei aCount, GLuint *aNames);
     void CreatedFramebuffers(GLContext *aOrigin, GLsizei aCount, GLuint *aNames);
     void CreatedRenderbuffers(GLContext *aOrigin, GLsizei aCount, GLuint *aNames);
     void DeletedProgram(GLContext *aOrigin, GLuint aName);
new file mode 100644
--- /dev/null
+++ b/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_
\ No newline at end of file
--- a/gfx/gl/SharedSurfaceGralloc.cpp
+++ b/gfx/gl/SharedSurfaceGralloc.cpp
@@ -137,19 +137,18 @@ SharedSurface_Gralloc::~SharedSurface_Gr
 void
 SharedSurface_Gralloc::Fence()
 {
     // We should be able to rely on genlock write locks/read locks.
     // But they're broken on some configs, and even a glFinish doesn't
     // 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);
     }
 }
 
 bool
 SharedSurface_Gralloc::WaitSync()
 {
     return true;
 }
--- a/gfx/gl/moz.build
+++ b/gfx/gl/moz.build
@@ -41,16 +41,17 @@ EXPORTS += [
     'GLLibraryEGL.h',
     'GLLibraryLoader.h',
     'GLReadTexImageHelper.h',
     'GLScreenBuffer.h',
     'GLSharedHandleHelpers.h',
     'GLTextureImage.h',
     'GLTypes.h',
     'GLUploadHelpers.h',
+    'HeapCopyOfStackArray.h',
     'ScopedGLHelpers.h',
     'SharedSurface.h',
     'SharedSurfaceEGL.h',
     'SharedSurfaceGL.h',
     'SurfaceFactory.h',
     'SurfaceStream.h',
     'SurfaceTypes.h',
     'TextureGarbageBin.h',
--- a/gfx/layers/opengl/CompositorOGL.cpp
+++ b/gfx/layers/opengl/CompositorOGL.cpp
@@ -39,16 +39,17 @@
 #include "nsMathUtils.h"                // for NS_roundf
 #include "nsRect.h"                     // for nsIntRect
 #include "nsServiceManagerUtils.h"      // for do_GetService
 #include "nsString.h"                   // for nsString, nsAutoCString, etc
 #include "DecomposeIntoNoRepeatTriangles.h"
 #include "ScopedGLHelpers.h"
 #include "GLReadTexImageHelper.h"
 #include "TiledLayerBuffer.h"           // for TiledLayerComposer
+#include "HeapCopyOfStackArray.h"
 
 #if MOZ_ANDROID_OMTC
 #include "TexturePoolOGL.h"
 #endif
 
 #include "GeckoProfiler.h"
 
 #if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17
@@ -367,17 +368,21 @@ CompositorOGL::Initialize()
   mGLContext->fBindBuffer(LOCAL_GL_ARRAY_BUFFER, mQuadVBO);
 
   GLfloat vertices[] = {
     /* First quad vertices */
     0.0f, 0.0f, 1.0f, 0.0f, 0.0f, 1.0f, 1.0f, 1.0f,
     /* 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>
     console(do_GetService(NS_CONSOLESERVICE_CONTRACTID));
 
   if (console) {
     nsString msg;
     msg +=