Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash when starting Grid3D plugin #235

Closed
wants to merge 1 commit into from

Conversation

darksylinc
Copy link
Contributor

If Grid3DPrivate::engine happens not to be a nullptr during
allocation, it will never be correctly initialized and will later try to
dereference an invalid ptr

Just in case we also initialize quickWindow although it doesn't seem to
be strictly necessary.

Signed-off-by: Matias N. Goldberg dark_sylinc@yahoo.com.ar

Summary

Self-explanatory. I ran into this bug by chance. Just open the Grid3D plugin from the GUI and it will crash (unless you're lucky that Grid3DPrivate::engine ended up being nullptr).

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

If `Grid3DPrivate::engine` happens not to be a nullptr during
allocation, it will never be correctly initialized and will later try to
dereference an invalid ptr

Just in case we also initialize quickWindow although it doesn't seem to
be strictly necessary.

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
@darksylinc darksylinc requested a review from chapulina as a code owner June 20, 2021 19:59
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Jun 20, 2021
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already fixed here. We need to forward port the commit.

@chapulina
Copy link
Contributor

Thanks for the PR! As @ahcorde mentioned, we just need to forward-port the fix, I'll decline this in favor of that.

The 3 ➡️ 4 port is in #234, then we can open a 4 ➡️ 5.

@chapulina chapulina closed this Jun 21, 2021
@chapulina
Copy link
Contributor

Done in #238

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants