Skip to content

Prevent infinite loop when only BOT_PREFIX passed#1557

Merged
sijis merged 2 commits intoerrbotio:masterfrom
davidamin:davidamin-fix-infinite-loop
Feb 23, 2022
Merged

Prevent infinite loop when only BOT_PREFIX passed#1557
sijis merged 2 commits intoerrbotio:masterfrom
davidamin:davidamin-fix-infinite-loop

Conversation

@davidamin
Copy link
Copy Markdown
Contributor

I have hit an error in which a message that contains only my bot prefix causes the bot to become unresponsive to any further messages. After a bit of debugging, I believe the problem is that text here becomes an empty string, which makes the value of i start as 0, then decrement (because the empty string is not in commands), leading to an infinite loop where i decreases forever.

I have confirmed that changing the comparison operation to <= fixes this case, without breaking other commands as far as I can tell.

@nzlosh
Copy link
Copy Markdown
Contributor

nzlosh commented Feb 16, 2022

As I understand the code, the proposed change will stop processing the first element of the command. I'm interested in reproducing the infinite loop issue.

Can you provide the following information so I can setup errbot as close to your setup please:

  • Execution environment
  • Errbot version
  • Chat backend
  • Bot Configuration
  • Command that triggers the loop

@davidamin
Copy link
Copy Markdown
Contributor Author

Sure thing! I've been able to reproduce this on version 6.1.8 using the text backend, with the default configuration defined by errbot --init.

To reproduce, I fire up the bot with errbot, then run a simple test command like !help. That returns as expected, but sending just ! as the command stops the bot from ever returning. I have been able to reproduce the same behavior with the Slack backend, and by running the bot in Docker.

@davidamin
Copy link
Copy Markdown
Contributor Author

Perhaps a better way to resolve the issue here would be by checking if the text string is empty? Happy to update the PR if that would be a cleaner approach

@nzlosh
Copy link
Copy Markdown
Contributor

nzlosh commented Feb 21, 2022

Sorry for the delay, I was able to reproduce the infinite loop as you described. This a really great catch, thank you for reporting this.

Your initial PR is a good solution. As a small bonus, if the test is moved from the bottom of the loop to the top and test for any value lower than 1, it will avoid the infinite loop as well as not spend time and resource to acquire the thread lock when the test_split list is empty.

            while cmd is None:
                if i < 1:
                    break

@nzlosh
Copy link
Copy Markdown
Contributor

nzlosh commented Feb 21, 2022

Moving the test to the top of the loop breaks code elsewhere in the function. Your initial fix is the right one. 👍

@sijis sijis force-pushed the davidamin-fix-infinite-loop branch from 15e7e94 to 7c97a75 Compare February 23, 2022 05:30
Copy link
Copy Markdown
Contributor

@sijis sijis left a comment

Choose a reason for hiding this comment

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

Nice catch. Thanks!

@sijis sijis merged commit 9aad157 into errbotio:master Feb 23, 2022
sijis added a commit that referenced this pull request Jun 11, 2022
* Prevent infinite loop when only BOT_PREFIX passed

* docs: add info to CHANGES

Co-authored-by: Sijis Aviles <sijis.aviles+github@gmail.com>
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.

3 participants