search

errbotio / errbot

Errbot is a chatbot, a daemon that connects to your favorite chat service and bring your tools and some fun into the conversation.
http://errbot.io
GNU General Public License v3.0
3.12k stars 612 forks source link

Prevent infinite loop when only BOT_PREFIX passed #1557

Closed davidamin closed 2 years ago

davidamin commented 2 years ago

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 commented 2 years ago

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:

davidamin commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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