fix: multiple crash and safety issues#1306
Open
markreidvfx wants to merge 7 commits into
Open
Conversation
Prevents a dangling pointer when the current session is destroyed. Remove the duplicate m_currentSession declaration from RvSession.h. The member is already declared in the base class Session.h, and the duplicate in the derived class shadows it. Signed-off-by: Mark Reid <markreid@netflixanimation.com>
Session::currentSession() can return null during teardown or before
a session is fully initialised. ~190 command entry points across
CommandsModule, MuUICommands, PyUICommands, and IPMu/CommandsModule
were dereferencing the pointer without checking, risking a null
pointer crash from any Mu or Python command invoked at the wrong
time.
Add null checks that throw a descriptive exception ("no current
session") before each use, turning undefined behaviour into a
catchable error.
Signed-off-by: Mark Reid <markreid@netflixanimation.com>
Signed-off-by: Mark Reid <markreid@netflixanimation.com>
The constructor did not initialize id or bufferId, so newly allocated objects contained whatever garbage the allocator returned. gcTextures() could then call glDeleteBuffers on these junk values, potentially destroying unrelated live GL buffers. Zero-initialize both fields in the constructor. Signed-off-by: Mark Reid <markreid@netflixanimation.com>
Signed-off-by: Mark Reid <markreid@netflixanimation.com>
Signed-off-by: Mark Reid <markreid@netflixanimation.com>
SessionIPNode called queryGLIntoContainer() during construction, which queries glGetString(GL_VERSION) etc. At that point, the correct GL context is not guaranteed to be current on the calling thread. In a multi-window process another window's context could be active, returning wrong GL capabilities or no valid context at all. Split property declaration from GL querying: declareGLProperties() registers all opengl.* slots with zero/empty defaults at construction time. The real values are populated later by queryAndStoreGLInfo() during the first render pass, where makeCurrent() ensures the correct context is active. Signed-off-by: Mark Reid <markreid@netflixanimation.com>
markreidvfx
requested review from
bernie-laberge,
cedrik-fuoco-adsk and
eloisebrosseau
as code owners
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issues
N/A
Summarize your change.
A collection of crash fixes and safety improvements found during internal testing and AddressSanitizer analysis. These are independent fixes grouped together for convenience.
Describe the reason for the change.
GL version error on startup:
SessionIPNodequeried GL state during construction before the correct context was current, returning wrong capabilities or crashing in multi-window setups.Dangling preferences window pointer: Non-owning raw pointer to the preferences window could dangle after the window was destroyed. Replaced with
QPointer.Dangling global settings pointer:
PackageManagerdid not null its global settings pointer after deletion.Uninitialized GL IDs in TextureDescription:
idandbufferIdwere not initialized, sogcTextures()could callglDeleteBufferson garbage values, destroying unrelated live GL buffers.Null order properties in PaintIPNode: Missing null check for order properties.
Null session pointer in command modules: ~190 command entry points dereferenced
Session::currentSession()without checking for null, risking crashes during teardown or before a session is fully initialized.Dangling session pointer after destruction:
Session::~Session()did not nullm_currentSessionwhen the active session was destroyed, leaving a dangling pointer. Also removes a duplicatem_currentSessiondeclaration fromRvSession.hthat shadowed the base class member.Describe what you have tested and on which operating system.
Tested on Rocky Linux 9. Several of these issues were identified with AddressSanitizer.
Add a list of changes, and note any that might need special attention during the review.
SessionIPNode.cpp: Defer GL queries to first render pass.RvApplication.h: UseQPointerfor preferences window reference.PackageManager.cpp: Null global settings pointer after deletion.ImageRenderer.h: Zero-initializeidandbufferIdinTextureDescription.PaintIPNode.cpp: Add null check for order properties.CommandsModule.cpp,MuUICommands.cpp,PyUICommands.cpp,IPMu/CommandsModule.cpp: Add null session checks to ~190 command entry points. This is the largest change and touches many functions.Session.cpp,RvSession.h: Nullm_currentSessionin destructor; remove duplicate declaration.If possible, provide screenshots.
N/A