Fix compiler error when -O2 optimzation is enabled

[djain1992]

Compiler error was flagged when the optimization was enabled

Description

Fix the possibility of buffer overflow causing compiler error.

Checklist:

• [x ] I have tested my changes. No regression in existing tests.
• My code is formatted using Uncrustify.
• [ x] I have read and applied the rules stated in CONTRIBUTING.md.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[kstribrnAmzn]

Taking a look at this PR. I'll likely be pushing one commit to it to fix the unit test failures.

[kstribrnAmzn]

Hello @djain1992! Thanks for the PR. Can you explain to me how the buffer overflow happens?

The previous code caps xThingNameLength, or the amount of the thingname that we copy, to a maximum of 53 characters.

xThingNameLength = ( uint32_t ) strnlen( ( const char * ) pAgentCtx->pThingName, OTA_CLIENT_TOKEN_MAX_THINGNAME_LEN );

With this change xThingNameLength is capped at 56 characters as the MSG_GET_NEXT_BUFFER_SIZE - msgSize on line 904 (in the changed version) will have a value of 56. This will allow the a thingname greater than 53 characters to take up the rest of the pMsg buffer. This will result in a malformed JSON payload.

[kstribrnAmzn]

Also can you tell me your compiler used and build steps? I tried to quickly reproduce this with the GNU 7.3.1 compiler with the following commands below but didn't see any error. I have CMake version cmake version 3.27.0-rc4 and make version GNU Make 3.82 installed.

cmake -S test -B build -DCMAKE_C_FLAGS='-O3 -Wall -Wextra -Werror'
make -C build

[djain1992]

Sorry to reply late.
I am using xtensa compiler which gets packages with esp32 SDK version 5.1

Thing name length can be utmost MSG_GET_NEXT_BUFFER_SIZE - msgSize. If MSG_GET_NEXT_BUFFER_SIZE - msgSize is less than OTA_CLIENT_TOKEN_MAX_THINGNAME_LEN, it will overwrite the msg buffer.

[kstribrnAmzn]

If MSG_GET_NEXT_BUFFER_SIZE - msgSize is less than OTA_CLIENT_TOKEN_MAX_THINGNAME_LEN, it will overwrite the msg buffer.

I agree with this statement. However, it is impossible for OTA_CLIENT_TOKEN_MAX_THINGNAME_LEN to be larger than MSG_GET_NEXT_BUFFER_SIZE - msgSize (aka the remaining buffer space).

By the time you hit this line at most the first 27 characters of the pMsg buffer are taken up. This buffer is sized to 83 characters on this line. This leaves 56 characters remaining. The final character is reserved for the null terminator. Then the thingname is added to the buffer (which is limited to 53 characters) followed by the "} characters to close the request object.

So at worst case the buffer is perfectly sized.

---

Update: full breakdown

characters 0->15 = `"{"clientToken":"`
characters 16->25 = This is the request ID which is a `uint32_t` as a string. This is a maximum of 10 characters.
character 26 = This is the colon separator
characters 27->79  = The thing name (maximum of 53 characers)
characters 80->81. = `"}`
character 82 = null character

[kstribrnAmzn]

With your change, the thingname can be at most MSG_GET_NEXT_BUFFER_SIZE - msgSize characters. Which if you follow the same pattern I laid out in the previous message is 56 characters. If you were to write this to the buffer then the message would actually become malformed since the final `"}" characters could not be added to the buffer.

[kstribrnAmzn]

Closing this PR as this will introduce a bug on thingnames which are longer than 53 characters. Feel free to reopen if you have a further issue.