Bug in UniversalTelegramBot::getUpdates(): cycling problem when deserializeJson happen to endup with an error

[RobertGnz]

There is a BUG in UniversalTelegramBot::getUpdates() function:

If deserializeJson happen to endup with an error for instance update to big for the buffer, then "last_message_received" is not updated and the function returns 0. Subsequent calls to getUpdates uses "last_message_received + 1" for the offset value which is passed to getUpdates. But as last_message_received was not updated, getUpdates will allways read the same message. deserializeJson will always endup with the same error and getUpdates will always return 0.

I propose the following changes in two places of UniversalTelegramBot::getUpdates. To make them easier to find in the code, changes begin with // Robert and ends with // End Robert

Please see below.

int UniversalTelegramBot::getUpdates(long offset) {

#ifdef TELEGRAM_DEBUG
Serial.println(F("GET Update Messages")); #endif String command = BOT_CMD("getUpdates?offset="); command += offset; command += F("&limit="); command += HANDLE_MESSAGES;

if (longPoll > 0) { command += F("&timeout="); command += String(longPoll); } String response = sendGetToTelegram(command); // receive reply from telegram.org

// Robert: // If deserializeJson happen to endup with an error for instance update to big for // the buffer, then last_message_received will not be updated. // Subsequent calls to getUpdates uses last_message_received + 1 for the offset value. // But as last_message_received was not updated we will allways read the same message. // Here we try to find the update_id and store it to update last_message_received if necessary. char * pt1; char * pt2; long candidate; candidate = -1;
pt1 = strstr_P( response.c_str(), (const char *) F("update_id"));
if ( pt1 != NULL ) { pt1 = strstr_P( pt1, (const char *) F(":")); if ( pt1 != NULL ) { pt1++;
pt2 = strstr_P( pt1, (const char *) F(",")); if ( pt2 != NULL ) { if ( pt2 - pt1 < 12 ) { // for safety
sscanf( pt1, "%ld", &candidate ); } } } }
// End Robert

if (response == "") { #ifdef TELEGRAM_DEBUG
Serial.println(F("Received empty string in response!")); #endif // close the client as there's nothing to do with an empty string closeClient(); return 0; } else { #ifdef TELEGRAM_DEBUG
Serial.print(F("incoming message length ")); Serial.println(response.length()); Serial.println(F("Creating DynamicJsonBuffer")); #endif

// Parse response into Json object
DynamicJsonDocument doc(maxMessageLength);
DeserializationError error = deserializeJson(doc, ZERO_COPY(response));
  
if (!error) {
  #ifdef TELEGRAM_DEBUG  
    Serial.print(F("GetUpdates parsed jsonObj: "));
    serializeJson(doc, Serial);
    Serial.println();
  #endif
  if (doc.containsKey("result")) {
    int resultArrayLength = doc["result"].size();
    if (resultArrayLength > 0) {
      int newMessageIndex = 0;
      // Step through all results
      for (int i = 0; i < resultArrayLength; i++) {
        JsonObject result = doc["result"][i];
        if (processResult(result, newMessageIndex)) newMessageIndex++;
      }
      // We will keep the client open because there may be a response to be
      // given
      return newMessageIndex;
    } else {
      #ifdef TELEGRAM_DEBUG  
        Serial.println(F("no new messages"));
      #endif
    }
  } else {
    #ifdef TELEGRAM_DEBUG  
        Serial.println(F("Response contained no 'result'"));
    #endif
  }
} else { // Parsing failed

  // Robert: try to update last_message_received 
  if ( candidate != -1 ) last_message_received = candidate;	  	  
  // End Robert
  
  if (response.length() < 2) { // Too short a message. Maybe a connection issue
    #ifdef TELEGRAM_DEBUG  
        Serial.println(F("Parsing error: Message too short"));
    #endif
  } else {
    // Buffer may not be big enough, increase buffer or reduce max number of
    // messages
    #ifdef TELEGRAM_DEBUG 
        Serial.print(F("Failed to parse update, the message could be too "
                       "big for the buffer. Error code: "));
        Serial.println(error.c_str()); // debug print of parsing error
    #endif     
  }
}
// Close the client as no response is to be given
closeClient();
return 0;

} }

[palmerabollo]

I think this is a critical issue because any user (even anonymous users, see #266) can kill any bot.

[francwalter]

Thank you very much, Robert! I changed your code and it works again. I had as test, if there wont be some overflow sent a 1500 char message and this blocked the code. Took me a while to find it (fist thing to find was the #define TELEGRAM_DEBUG to get the message_id of my bad message. Then I tried to delete the message with some API command deleteMessage:

https://api.telegram.org/bot0987654321:ABCDEF-abcdefghijklmnopqrstuvwxyzab/deleteMessage?chat_id=1234567890&message_id=2105

but even message_id and chat_id was correct, I got only a:

{"ok":false,"error_code":400,"description":"Bad Request: message to delete not found"}

I could not delete the message like that, I dont know why. Neither worked editMessageText. Before I deleted the bad long message in Telegram, where I had sent it, checked delete for both, but this didnt delete it. I could still read it with:

https://api.telegram.org/bot0987654321:ABCDEF-abcdefghijklmnopqrstuvwxyzab/getUpdates?offset=0&limit=1

So I found your code for the file: C:\Users\f\Documents\Arduino\libraries\Universal-Arduino-Telegram-Bot-master\src\UniversalTelegramBot.cpp changed and compiled it, and good it was again 👍 Here your code change again, within the whole getUpdates-function, formatted (I had a bit difficulties to find the second part of your changes, because unformatted, partly):

/***************************************************************
 * GetUpdates - function to receive messages from telegram *
 * (Argument to pass: the last+1 message to read)             *
 * Returns the number of new messages           *
 ***************************************************************/
int UniversalTelegramBot::getUpdates(long offset) {

  #ifdef TELEGRAM_DEBUG  
    Serial.println(F("GET Update Messages"));
  #endif
  String command = BOT_CMD("getUpdates?offset=");
  command += offset;
  command += F("&limit=");
  command += HANDLE_MESSAGES;

  if (longPoll > 0) {
    command += F("&timeout=");
    command += String(longPoll);
  }
  String response = sendGetToTelegram(command); // receive reply from telegram.org

// Robert:
// If deserializeJson happen to endup with an error for instance update to big for
// the buffer, then last_message_received will not be updated.
// Subsequent calls to getUpdates uses last_message_received + 1 for the offset value.
// But as last_message_received was not updated we will allways read the same message.
// Here we try to find the update_id and store it to update last_message_received if necessary.
  char * pt1; char * pt2; long candidate;
  candidate = -1;
  pt1 = strstr_P( response.c_str(), (const char *) F("update_id"));
  if ( pt1 != NULL ) {
    pt1 = strstr_P( pt1, (const char *) F(":"));
    if ( pt1 != NULL ) {
      pt1++;
      pt2 = strstr_P( pt1, (const char *) F(","));
      if ( pt2 != NULL ) {
        if ( pt2 - pt1 < 12 ) { // for safety 
          sscanf( pt1, "%ld", &candidate );
        }
      }
    }
  }
// End Robert
  if (response == "") {
    #ifdef TELEGRAM_DEBUG  
        Serial.println(F("Received empty string in response!"));
    #endif
    // close the client as there's nothing to do with an empty string
    closeClient();
    return 0;
  } else {
    #ifdef TELEGRAM_DEBUG  
      Serial.print(F("incoming message length "));
      Serial.println(response.length());
      Serial.println(F("Creating DynamicJsonBuffer"));
    #endif

    // Parse response into Json object
    DynamicJsonDocument doc(maxMessageLength);
    DeserializationError error = deserializeJson(doc, ZERO_COPY(response));
      
    if (!error) {
      #ifdef TELEGRAM_DEBUG  
        Serial.print(F("GetUpdates parsed jsonObj: "));
        serializeJson(doc, Serial);
        Serial.println();
      #endif
      if (doc.containsKey("result")) {
        int resultArrayLength = doc["result"].size();
        if (resultArrayLength > 0) {
          int newMessageIndex = 0;
          // Step through all results
          for (int i = 0; i < resultArrayLength; i++) {
            JsonObject result = doc["result"][i];
            if (processResult(result, newMessageIndex)) newMessageIndex++;
          }
          // We will keep the client open because there may be a response to be
          // given
          return newMessageIndex;
        } else {
          #ifdef TELEGRAM_DEBUG  
            Serial.println(F("no new messages"));
          #endif
        }
      } else {
        #ifdef TELEGRAM_DEBUG  
            Serial.println(F("Response contained no 'result'"));
        #endif
      }
    } else { // Parsing failed
      // Robert: try to update last_message_received 
      if ( candidate != -1 ) last_message_received = candidate;	  	  
      // End Robert
	  
      if (response.length() < 2) { // Too short a message. Maybe a connection issue
        #ifdef TELEGRAM_DEBUG  
            Serial.println(F("Parsing error: Message too short"));
        #endif
      } else {
        // Buffer may not be big enough, increase buffer or reduce max number of
        // messages
        #ifdef TELEGRAM_DEBUG 
            Serial.print(F("Failed to parse update, the message could be too "
                           "big for the buffer. Error code: "));
            Serial.println(error.c_str()); // debug print of parsing error
        #endif     
      }
    }
    // Close the client as no response is to be given
    closeClient();
    return 0;
  }
}

[francwalter]

@RobertGnz By the way: I made a pull request with your code, put a deleteMessage command as well.