From 06bae26e1b6b4b28b9fc1d95ac75700add1132fc Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Wed, 29 Nov 2023 09:39:35 -0800 Subject: [PATCH] better handle invalid programs in release builds (#7389) * better handle invalid programs in release builds Until now invalid program would basically be undefined behavior, which in practice was a crash via a null pointer dereference. With this change, invalid programs cause drawing ops to become no-ops. Additionally fixed an unsynchronized access of a the variable containing the program id. I don't think it would have caused issues though. FIXES=[311775564] Co-authored-by: Powei Feng --- filament/backend/src/opengl/OpenGLDriver.cpp | 38 ++++++++++--------- filament/backend/src/opengl/OpenGLDriver.h | 2 +- filament/backend/src/opengl/OpenGLProgram.cpp | 2 +- filament/backend/src/opengl/OpenGLProgram.h | 2 +- .../src/opengl/ShaderCompilerService.cpp | 7 +--- 5 files changed, 26 insertions(+), 25 deletions(-) diff --git a/filament/backend/src/opengl/OpenGLDriver.cpp b/filament/backend/src/opengl/OpenGLDriver.cpp index 1c49dcc0ecff..83b20156f32d 100644 --- a/filament/backend/src/opengl/OpenGLDriver.cpp +++ b/filament/backend/src/opengl/OpenGLDriver.cpp @@ -262,7 +262,12 @@ void OpenGLDriver::bindTexture(GLuint unit, GLTexture const* t) noexcept { mContext.bindTexture(unit, t->gl.target, t->gl.id, t->gl.targetIndex); } -void OpenGLDriver::useProgram(OpenGLProgram* p) noexcept { +bool OpenGLDriver::useProgram(OpenGLProgram* p) noexcept { + if (UTILS_UNLIKELY(!p->isValid())) { + // If the program is not valid, we can't call use(). + return false; + } + // set-up textures and samplers in the proper TMUs (as specified in setSamplers) p->use(this, mContext); @@ -277,6 +282,7 @@ void OpenGLDriver::useProgram(OpenGLProgram* p) noexcept { // when mPlatform.isSRGBSwapChainSupported() is false (no need to check though). p->setRec709ColorSpace(mRec709OutputColorspace); } + return true; } @@ -3531,18 +3537,17 @@ void OpenGLDriver::draw(PipelineState state, Handle rph, uint DEBUG_MARKER() auto& gl = mContext; - OpenGLProgram* p = handle_cast(state.program); + OpenGLProgram* const p = handle_cast(state.program); - // If the material debugger is enabled, avoid fatal (or cascading) errors and that can occur - // during the draw call when the program is invalid. The shader compile error has already been - // dumped to the console at this point, so it's fine to simply return early. - if (FILAMENT_ENABLE_MATDBG && UTILS_UNLIKELY(!p->isValid())) { + bool const success = useProgram(p); + if (UTILS_UNLIKELY(!success)) { + // Avoid fatal (or cascading) errors that can occur during the draw call when the program + // is invalid. The shader compile error has already been dumped to the console at this + // point, so it's fine to simply return early. return; } - useProgram(p); - - GLRenderPrimitive* rp = handle_cast(rph); + GLRenderPrimitive* const rp = handle_cast(rph); // Gracefully do nothing if the render primitive has not been set up. VertexBufferHandle vb = rp->gl.vertexBufferWithObjects; @@ -3553,7 +3558,7 @@ void OpenGLDriver::draw(PipelineState state, Handle rph, uint gl.bindVertexArray(&rp->gl); // If necessary, mutate the bindings in the VAO. - const GLVertexBuffer* glvb = handle_cast(vb); + GLVertexBuffer const* const glvb = handle_cast(vb); if (UTILS_UNLIKELY(rp->gl.vertexBufferVersion != glvb->bufferObjectsVersion)) { updateVertexArrayObject(rp, glvb); } @@ -3587,17 +3592,16 @@ void OpenGLDriver::draw(PipelineState state, Handle rph, uint void OpenGLDriver::dispatchCompute(Handle program, math::uint3 workGroupCount) { getShaderCompilerService().tick(); - OpenGLProgram* p = handle_cast(program); + OpenGLProgram* const p = handle_cast(program); - // If the material debugger is enabled, avoid fatal (or cascading) errors and that can occur - // during the draw call when the program is invalid. The shader compile error has already been - // dumped to the console at this point, so it's fine to simply return early. - if (FILAMENT_ENABLE_MATDBG && UTILS_UNLIKELY(!p->isValid())) { + bool const success = useProgram(p); + if (UTILS_UNLIKELY(!success)) { + // Avoid fatal (or cascading) errors that can occur during the draw call when the program + // is invalid. The shader compile error has already been dumped to the console at this + // point, so it's fine to simply return early. return; } - useProgram(p); - #if defined(BACKEND_OPENGL_LEVEL_GLES31) #if defined(__ANDROID__) diff --git a/filament/backend/src/opengl/OpenGLDriver.h b/filament/backend/src/opengl/OpenGLDriver.h index f3c87c617686..04e003fb3da9 100644 --- a/filament/backend/src/opengl/OpenGLDriver.h +++ b/filament/backend/src/opengl/OpenGLDriver.h @@ -318,7 +318,7 @@ class OpenGLDriver final : public DriverBase { void bindTexture(GLuint unit, GLTexture const* t) noexcept; void bindSampler(GLuint unit, GLuint sampler) noexcept; - inline void useProgram(OpenGLProgram* p) noexcept; + inline bool useProgram(OpenGLProgram* p) noexcept; enum class ResolveAction { LOAD, STORE }; void resolvePass(ResolveAction action, GLRenderTarget const* rt, diff --git a/filament/backend/src/opengl/OpenGLProgram.cpp b/filament/backend/src/opengl/OpenGLProgram.cpp index 69412e1aea14..aef088f60cf2 100644 --- a/filament/backend/src/opengl/OpenGLProgram.cpp +++ b/filament/backend/src/opengl/OpenGLProgram.cpp @@ -129,7 +129,7 @@ void OpenGLProgram::initializeProgramState(OpenGLContext& context, GLuint progra #endif { // ES2 initialization of (fake) UBOs - UniformsRecord* const uniformsRecords = new UniformsRecord[Program::UNIFORM_BINDING_COUNT]; + UniformsRecord* const uniformsRecords = new(std::nothrow) UniformsRecord[Program::UNIFORM_BINDING_COUNT]; UTILS_NOUNROLL for (GLuint binding = 0, n = Program::UNIFORM_BINDING_COUNT; binding < n; binding++) { Program::UniformInfo& uniforms = lazyInitializationData.bindingUniformInfo[binding]; diff --git a/filament/backend/src/opengl/OpenGLProgram.h b/filament/backend/src/opengl/OpenGLProgram.h index ddee9496690e..5d74fdc827de 100644 --- a/filament/backend/src/opengl/OpenGLProgram.h +++ b/filament/backend/src/opengl/OpenGLProgram.h @@ -59,7 +59,7 @@ class OpenGLProgram : public HwProgram { // - the content of any bound sampler buffer has changed // ... since last time we used this program - // turns out the former might be relatively cheap to check, the later requires + // Turns out the former might be relatively cheap to check, the latter requires // a bit less. Compared to what updateSamplers() actually does, which is // pretty little, I'm not sure if we'll get ahead. diff --git a/filament/backend/src/opengl/ShaderCompilerService.cpp b/filament/backend/src/opengl/ShaderCompilerService.cpp index 9ab30a9bf10d..e816b0161d90 100644 --- a/filament/backend/src/opengl/ShaderCompilerService.cpp +++ b/filament/backend/src/opengl/ShaderCompilerService.cpp @@ -22,7 +22,6 @@ #include -#include #include #include @@ -254,8 +253,6 @@ ShaderCompilerService::program_token_t ShaderCompilerService::createProgram( glGetProgramiv(glProgram, GL_LINK_STATUS, &status); programData.program = glProgram; - token->gl.program = programData.program; - // we don't need to check for success here, it'll be done on the // main thread side. token->set(programData); @@ -337,12 +334,12 @@ GLuint ShaderCompilerService::getProgram(ShaderCompilerService::program_token_t& return program; } -/* static*/ void ShaderCompilerService::terminate(program_token_t& token) { +void ShaderCompilerService::terminate(program_token_t& token) { assert_invariant(token); token->canceled = true; - bool canceled = token->compiler.cancelTickOp(token); + bool const canceled = token->compiler.cancelTickOp(token); if (token->compiler.mShaderCompilerThreadCount) { auto job = token->compiler.mCompilerThreadPool.dequeue(token);