From 7d4874a62065e9dd5dacf5f12c4d1a9aafc02d4c Mon Sep 17 00:00:00 2001 From: Benoit Jacob Date: Thu, 8 May 2014 21:03:37 -0400 Subject: [PATCH] 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 | 33 +++++++++++++++++++- gfx/gl/GLContext.h | 30 ++++++++++++------ gfx/gl/HeapCopyOfStackArray.h | 47 +++++++++++++++++++++++++++++ gfx/gl/SharedSurfaceGralloc.cpp | 5 ++- gfx/gl/moz.build | 1 + gfx/layers/opengl/CompositorOGL.cpp | 7 ++++- 6 files changed, 108 insertions(+), 15 deletions(-) create mode 100644 gfx/gl/HeapCopyOfStackArray.h diff --git a/gfx/gl/GLContext.cpp b/gfx/gl/GLContext.cpp index 1bc817a8fd8..22b242fe1fc 100644 --- a/gfx/gl/GLContext.cpp +++ b/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(ptr) >> page_bits; + intptr_t someStackPage = reinterpret_cast(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) { diff --git a/gfx/gl/GLContext.h b/gfx/gl/GLContext.h index e7ef4e6a832..71a50f388f6 100644 --- a/gfx/gl/GLContext.h +++ b/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 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); diff --git a/gfx/gl/HeapCopyOfStackArray.h b/gfx/gl/HeapCopyOfStackArray.h new file mode 100644 index 00000000000..a06f87dc6b1 --- /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 + +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 +class HeapCopyOfStackArray +{ +public: + template + 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 const mArrayData; +}; + +} + +#endif // HEAPCOPYOFSTACKARRAY_H_ \ No newline at end of file diff --git a/gfx/gl/SharedSurfaceGralloc.cpp b/gfx/gl/SharedSurfaceGralloc.cpp index 4c3b8cb9aa1..f132ae5ca62 100644 --- a/gfx/gl/SharedSurfaceGralloc.cpp +++ b/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 buf(new char[4]); + mGL->fReadPixels(0, 0, 1, 1, LOCAL_GL_RGBA, LOCAL_GL_UNSIGNED_BYTE, buf); } } diff --git a/gfx/gl/moz.build b/gfx/gl/moz.build index adbdf3661ec..079312cd473 100644 --- a/gfx/gl/moz.build +++ b/gfx/gl/moz.build @@ -46,6 +46,7 @@ EXPORTS += [ 'GLTextureImage.h', 'GLTypes.h', 'GLUploadHelpers.h', + 'HeapCopyOfStackArray.h', 'ScopedGLHelpers.h', 'SharedSurface.h', 'SharedSurfaceEGL.h', diff --git a/gfx/layers/opengl/CompositorOGL.cpp b/gfx/layers/opengl/CompositorOGL.cpp index c3f157a6f7e..47f96de7a72 100644 --- a/gfx/layers/opengl/CompositorOGL.cpp +++ b/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 verticesOnHeap(vertices); + mGLContext->fBufferData(LOCAL_GL_ARRAY_BUFFER, + verticesOnHeap.ByteLength(), + verticesOnHeap.Data(), + LOCAL_GL_STATIC_DRAW); mGLContext->fBindBuffer(LOCAL_GL_ARRAY_BUFFER, 0); nsCOMPtr