dani007200964 / Commander-API

Commander-API is a simple to use C++ parser library and you can easily use it to process character based commands
MIT License
24 stars 4 forks source link

`executeCommand` copies string without null terminator #19

Open ondras12345 opened 1 year ago

ondras12345 commented 1 year ago

Describe the bug https://github.com/dani007200964/Commander-API/blob/9d185dcbdbec260cebfd8d8f1204752a47d0b76a/src/Commander-API.cpp#L318-L320

$ man strncpy ... The strncpy() function is similar, except that at most n bytes of src are copied. Warning: If there is no null byte among the first n bytes of src, the string placed in dest will not be null-terminated.

If cmd is longer than COMMANDER_MAX_COMMAND_SIZE, strncpy will not terminate the string in tempBuff. The next function call to hasChar will continue reading the string until it finds a null character, thus reading past the end of tempBuff.

I have done some very limited testing and it seems that the byte immediately following tempBuff in memory happens to be 0x00 in my case. However, this is not guaranteed.

tempBuff[COMMANDER_MAX_COMMAND_SIZE - 1] == 0x39  // '9'
tempBuff[COMMANDER_MAX_COMMAND_SIZE] == 0x00 // past the end of the buffer!

A quick fix could be something like this:

strncpy( tempBuff, cmd, COMMANDER_MAX_COMMAND_SIZE );
tempBuff[sizeof(tempBuff)-1] = '\0';  // sizeof(tempBuff) == COMMANDER_MAX_COMMAND_SIZE

However, this would result in a maximum command size of COMMANDER_MAX_COMMAND_SIZE - 1. If you wanted to keep the max command size, you would need to make tempBuff 1 byte longer. https://github.com/dani007200964/Commander-API/blob/9d185dcbdbec260cebfd8d8f1204752a47d0b76a/src/Commander-API.hpp#L258-L262 Warning: I have not actually tested this fix.

dani007200964 commented 1 year ago

Good catch, I started working on the fix.