app: Fix stack corruption with binary to hex conversion#173
Conversation
MarkusLassila
requested review from
Copilot,
juhaylinen,
kacperradoszewski and
trantanen
| } | ||
|
|
||
| for (int i = 0; i < hex_len; i++) { | ||
| sprintf(ascii + (i * 2), "%02X", *(hex + i)); |
There was a problem hiding this comment.
This caused the stack corruption. When filling the whole buffer, the terminating null-character was written beyond the buffer.
There was a problem hiding this comment.
Pull request overview
Fixes a stack corruption issue caused by sprintf writing a '\0' past the end of the destination buffer during binary↔hex conversions, and updates call sites accordingly.
Changes:
- Reworked binary-to-hex and hex-to-binary conversion helpers and renamed the APIs.
- Tightened hex-string validation and replaced
sprintf/strncpy/strtoulconversions with manual nibble conversion. - Updated socket and carrier codepaths to use the new conversion helpers and buffers.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| app/src/sm_util.h | Renames/clarifies hex helpers and updates API documentation. |
| app/src/sm_util.c | Replaces sprintf-based conversion with manual conversion and updates validation. |
| app/src/sm_at_socket.c | Switches socket send paths to use the new bin2hex/hex2bin helpers. |
| app/src/lwm2m_carrier/sm_at_carrier.c | Switches carrier app-data paths to the new conversion helpers and variable naming. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Reworking this to use zephyr defaults. |
MarkusLassila
force-pushed
the
fix-stack-corruption
branch
from
b13b5bd to
47a3a15
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MarkusLassila
force-pushed
the
fix-stack-corruption
branch
from
47a3a15 to
0d03740
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int fd; | ||
| uint16_t mode; | ||
| int size; | ||
| size_t size; |
There was a problem hiding this comment.
do_send() (per context) takes const uint8_t *data and an int len, but this code passes a const char * (str_ptr) and a size_t (size). This can trigger incompatible-pointer and narrowing conversion warnings and risks truncation if sizes ever exceed INT_MAX. Prefer keeping a const uint8_t *data_ptr for the payload buffer (cast once where needed), and either change do_send()/do_sendto() to accept size_t lengths or add an explicit, bounded cast at the call site.
| size_t size; | |
| int size; |
There was a problem hiding this comment.
size to be dealt in later PR.
MarkusLassila
force-pushed
the
fix-stack-corruption
branch
from
0d03740 to
9a0640f
Compare
|
@kacperradoszewski: We have to merge this today. Feel free to comment later on the sm_at_carrier.c parts. |
sm_util_htoa used sprintf to convert from binary to hex. With data sizes exceeding the buffer, the null-character was written beyond allocated space. In this particular case the socket pointer was corrupted. sm_util_htoa and sm_util_atoh replaced with bin2hex and hex2bin. Signed-off-by: Markus Lassila <markus.lassila@nordicsemi.no>
MarkusLassila
force-pushed
the
fix-stack-corruption
branch
from
9a0640f to
c0ea725
Compare
Sorry, had to prioritize other stuff today. I did a quick sanity check now and everything seemed ok 👍 |
sm_util_htoa used sprintf to convert from binary to hex.
With data sizes exceeding the buffer, the null-character was
written beyond allocated space. In this particular case the
socket pointer was corrupted.
sm_util_htoa and sm_util_atoh replaced with bin2hex and hex2bin.
Jira: SM-245