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

automod: recover from panics in applying effects #1695

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

jo3-l
Copy link
Contributor

@jo3-l jo3-l commented Jul 4, 2024

This PR is one of several aiming to mitigate the impact of bugs similar to those introduced in the previous release.

The most pressing issue was the inadvertent removal of a nil guard in KickUser by me in #1659, which caused panics which were difficult to track down. To make these easier to debug in future (and to prevent complete bot crashes), in this PR we recover and log a stacktrace for panics in applying advanced automod effects and basic automod punishments. This supercedes the temporary bandaid recover added directly to KickUser in f9602a1.

For reference, I compiled a version of the bot with the nil pointer bug re-added (but with this patch applied as well.) Instead of crashing, the following is logged instead:

ERRO recovered from panic applying automod effect
runtime error: invalid memory address or nil pointer dereference
goroutine 379 [running]:
runtime/debug.Stack()
        /usr/local/go/src/runtime/debug/stack.go:24 +0x5e
github.com/botlabs-gg/yagpdb/v2/automod.(*Plugin).RulesetRulesTriggeredCondsPassed.func1.1()
        /REDACTED/yagpdb/automod/automod_bot.go:477 +0x38
panic({0x14a1cc0?, 0x492e600?})
        /usr/local/go/src/runtime/panic.go:770 +0x132
github.com/botlabs-gg/yagpdb/v2/moderation.KickUser(0x7ffa19f13270?, 0xad7ea432382000a, 0x0, 0x0, 0xc0000f8480, {0xc001908de0, 0x18}, 0xc00065ea00, 0xffffffffffffffff)
         /REDACTED/yagpdb/moderation/punishments.go:236 +0xf1
github.com/botlabs-gg/yagpdb/v2/automod.(*KickUserEffect).Apply(0x4931210?, 0xc0003f7860, {0x13b8c20?, 0xc000399dd0?})
         /REDACTED/yagpdb/automod/effects.go:272 +0xb2
github.com/botlabs-gg/yagpdb/v2/automod.(*Plugin).RulesetRulesTriggeredCondsPassed.func1(0xc00053bf00, 0xc0003f7860?)
         /REDACTED/yagpdb/automod/automod_bot.go:482 +0xf2
created by github.com/botlabs-gg/yagpdb/v2/automod.(*Plugin).RulesetRulesTriggeredCondsPassed in goroutine 473
        /REDACTED/yagpdb/automod/automod_bot.go:474 +0xd68  p=automod_v2 stck="automod.(*Plugin).RulesetRulesTriggeredCondsPassed.func1.1:automod_bot.go:478"

which points directly to the problematic line.

Comment on lines -220 to -224
defer func() {
if r := recover(); r != nil {
logger.Infof("Recovered from panic: %#v", r)
}
}()
Copy link
Contributor Author

@jo3-l jo3-l Jul 4, 2024

Choose a reason for hiding this comment

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

This recover is completely safe to remove given the other patches in this PR. KickUser is called from three locations in the source:

  • once in the kick command (commands.go:313)
    • We already recover from panics in command executions.
  • once in automod legacy (automod_legacy/bot.go:167)
    • The necessary panic handler is added in this PR.
  • and once in advanced automod via the kick user effect (automod/effects.go:272).
    • The necessary panic handler is added in this PR.

hence, there is no need for a separate recover directly in KickUser anymore.

@ashishjh-bst ashishjh-bst merged commit d09ece5 into botlabs-gg:dev Jul 10, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants