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

[MM-54551] Fix potential nil dereference panic #532

Merged
merged 1 commit into from
Sep 29, 2023
Merged

[MM-54551] Fix potential nil dereference panic #532

merged 1 commit into from
Sep 29, 2023

Conversation

streamer45
Copy link
Collaborator

Summary

We had a report of failures to join calls starting in v0.19. At first it looked like a networking issue but upon further inspection of the logs we were able to find multiple plugin crashes due to SIGSEGV.

Even if properly set to false in config.json the returned object was a nil pointer. Adding a couple of extra checks to avoid future regressions.

Given the severity of this I'd like to cut a new dot release once this is merged.

Ticket Link

https://mattermost.atlassian.net/browse/MM-54551

@streamer45 streamer45 added the 2: Dev Review Requires review by a core committer label Sep 25, 2023
@streamer45 streamer45 added this to the v0.19.2 milestone Sep 25, 2023
@streamer45 streamer45 requested a review from cpoile September 25, 2023 18:15
@streamer45 streamer45 self-assigned this Sep 25, 2023
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

Huh. Guess we do need to always check that. Thanks!

@cpoile
Copy link
Member

cpoile commented Sep 26, 2023

Any idea why it's returning a nil config?

@streamer45
Copy link
Collaborator Author

Any idea why it's returning a nil config?

Not sure but I think the returned setting is nil, not the config because I had the reporting user setting it to true instead which fixed the crashes.

@streamer45
Copy link
Collaborator Author

Also to note, what made debugging this worse was that until we enabled DEBUG level logs we couldn't get the plugin panics to show. Something to review on the server most likely.

@streamer45
Copy link
Collaborator Author

If we're looking at that, then we should note that Plugin panics should be errors in the first place.

Yeah, currently it's something handled by the framework (https://mattermost.atlassian.net/browse/MM-54562), unless we start to implement recovers, something I tried to avoid so far.

@streamer45 streamer45 added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Sep 26, 2023
@streamer45 streamer45 merged commit c43feb8 into main Sep 29, 2023
@streamer45 streamer45 deleted the MM-54551 branch September 29, 2023 15:05
cpoile pushed a commit that referenced this pull request Oct 2, 2023
@cpoile cpoile mentioned this pull request Oct 2, 2023
cpoile added a commit that referenced this pull request Oct 2, 2023
* [MM-54520] Soft fail in case of missing post during state cleanup (#530)

* Soft fail in case of missing post during state cleanup

* Add validity check on postID

* Fix potential nil dereference panic (#532)

---------

Co-authored-by: Claudio Costa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants