CWE-628: Function Call with Incorrectly Specified Arguments

[aly-fly]

While running the code through PIO Inspector, this incompliance was found.
File: Mqtt_client_ips.cpp : 146 : 34
Function: void callback(char* topic, byte* payload, unsigned int length) { //A new message has been received

Code:

    sprintf(message, "%c", (char)payload[0]);
    for (int i = 1; i < length; i++) {
        sprintf(message, "%s%c", message, (char)payload[i]);

cppcheck	HIGH	ERROR	CWE-628:Undefined behavior: Variable 'message' is used as parameter and destination in sprintf().

Same array "message" is used as input and output of the function. As far as I understand, this loop just copies arrays "payload" into "message". One by one, byte into char. It surely can be optimized to avoid using sprintf, right?

[Martinius79]

Yes, it just copies the payload content into the message buffer and converts it from byte to char.
I don't see any reason to use a byte-by-byte copy mechanism here because byte is char and vice versa for this platform.

Maybe there is a speed or memory usage improvment on ESPs over using a std lib function...

The beginning of a refactored function of the actual main with some comments would look like this:

void callback(char* topic, byte* payload, unsigned int length) {  // A new message has been received
  //find out how many "commands" are in the topic
  int commandNumber = 10;                                       // set a maximum "command depth" (each command is separated by "/" and is a token in the array after splitting)
  char* command[commandNumber];                                 // initalize an array of pointers for the splitted topic (commands then) (will be filled by the splitCommand function)
  commandNumber = splitCommand(topic, command, commandNumber);  // split the topic of the received message into tokens (commands) and return the number of commands found - removes the first part of the topic (MQTT_CLIENT)
  
  // cast the payload to a char array (string)
  char message[length + 1];                 // initialize buffer to cast the payload from byte to char (message "content" string). Size is length + 1 for the null terminator
  strncpy(message, (char*)payload, length); // copy the payload to the message buffer (cast from byte to char) - strncpy is used to prevent buffer overflow
  message[length] = '\0';                   // add the zero-terminator to the end of the message string (always write the last byte of the buffer with null terminator)

  // check if the number of commands is at least 2 (e.g. topic = "device/directive/state" = 3 tokens. After splitCommands: 2 commands = directive + state)
  // if tokensNumber < 2, the code below crashes on the strcmp on non-initialized pointers in command[] array (i.e. directly use command[1])
  if (commandNumber < 2) {
      Serial.println("Detected number of commands in MQTT message is lower then 2! -> Ignoring message because it is not valid!");
      return;
  }

  // print the full topic and the received message to the serial monitor
  Serial.println();
  Serial.print("RX MQTT: ");
  Serial.print(topic);
  Serial.print(" ");
  Serial.println(message);

  #ifndef MQTT_HOME_ASSISTANT
  //------------------Decide what to do depending on the topic and message---------------------------------
  // Only two commands are supported: "directive/powerState" and ()"directive/setpoint" or "directive/percentage")
...

Tested and working for MQTT_ENABLED and MQTT_HOME_ASSISTANT

[aly-fly]

Cool, thanks for the heads-up. Can you please run this new code through the PIO inspector and if everything is fine, commit it? Thanks.

[Martinius79]

Sure, It was already included in the last PR (#95)

I ran the PlatformIO Inspector again then and there was some more stuff... It actually just runs the command "platformio.exe check --environment esp32dev" in the background, so I prefer the command-line output myself.

I fixed, the rest of the "fixable" findings in our code.

I created a brach for it, in the moment just in my fork: https://github.com/Martinius79/EleksTubeHAX/tree/FixCPPCheckFindings

The result is:

Component	HIGH	MEDIUM	LOW
.pio\libdeps\esp32dev\Adafruit NeoPixel	0	0	7
.pio\libdeps\esp32dev\ArduinoJson\src\ArduinoJson\Polyfills	2	0	0
.pio\libdeps\esp32dev\TFT_eSPI	0	0	8
.pio\libdeps\esp32dev\TFT_eSPI\Extensions	0	0	64
src	0	0	20
Total	2	0	99

Environment	Tool	Status	Duration
esp32dev	cppcheck	PASSED	00:02:12.247

This looks, like it there are still some findings in our code (src = 20 low), but if you take a closer look and understand the limits of the cppcheck mechanisms, you'll see that these are all "false positives" and warnings about "unusedFunction," which isn't true.. cppcheck ist just limited how it checks the calls from other classes. So it's unable to detect that the methods are actually used (most of them).

All other findings are not fixable for us because they are in the external libraries we use. Some of them are also false positives, like the high ones (macro preprocessor of cppcheck is limited).

A complete list, is attached:
ListOfCppcheckFindings.txt

So I guess, that is all we can "fix" for now...

Two Questions now:

• Should I make a PR to integrate the changes form the mentioned branch? Or you can do it yourself, you have full access to all forks.

• Do we want to try to format all the documents with the "default" formatting set of the Visual Studio Code C++ Extension? This is just an idea, not yet done in this branch.

Let me know your thoughts.

[aly-fly]

Hi, thanks.

1. I quickly reviewed the branch, it looks okay. PR is recommended.
2. Source files are from multiple authors and multiple IDE environments - hence different styles. This will change most if not all files. It would be useful to do it, as a separate commit.

[Martinius79]

I created a PR for the cpp check finding fixes #101

The format stuff is not that important, I guess...I prepeared a branch already, but need to integrate the latest changes to it.

[aly-fly]

Thanks!