Infrared: initialize timings_cnt on decoder alloc and fix its bounds check - #4429
Conversation
…check infrared_common_decoder_alloc() sets protocol and level but leaves timings_cnt uninitialized. Every caller allocs then resets, and for RC5 (the one protocol with preamble_mark == 0) the reset path reads timings_cnt and feeds it to consume_samples() as a length before the reset function zeroes it a few lines later. With a nonzero garbage value that shifts the 6-element timings[] array out of bounds, both read and write. Separately, the bounds check in infrared_common_decode() compares timings_cnt (an element count) against sizeof(decoder->timings) (24 bytes for six uint32_t), so it doesn't fire until 24 instead of 6. Should be COUNT_OF. Both come down to timings_cnt not being trustworthy; init it at alloc and use the element count in the check. Signed-off-by: Cole Munz <colemunz@gmail.com>
|
Hi @munzzyy I have two questions, just curious |
turbospok
left a comment
There was a problem hiding this comment.
I agree on furi_check fix
However the statement about uninitialised timings_cnt is wrong
Flipper's malloc always returns zeroed memory. timings_cnt is 0 at alloc; the if(decoder->timings_cnt > 0) guard in reset_state never fires.
So setting it to zero at alloc is just a visual change, not affecting anything on hardware, that seems to be your testing issue
Ill keep it just for better readability
|
You're right, I checked after your comment. |
What's new
Two small issues around
timings_cntin the common IR decoder.infrared_common_decoder_alloc()setsprotocolandlevelbut leavestimings_cntuninitialized. Every caller goes throughinfrared_alloc_decoder(), which allocs then immediately resets. For RC5 (the one protocol withpreamble_mark == 0), the reset path readstimings_cntand passes it toconsume_samples()as a length, and the reset function only zeroestimings_cnta couple of lines later, afterreset_state()has already used it. If the uninitialized value happens to be nonzero,consume_samples()shifts the 6-elementtimings[]array using that length, reading and writing past the end.Separately, the defensive check in
infrared_common_decode()isfuri_check(decoder->timings_cnt <= sizeof(decoder->timings)).timings_cntis an element count butsizeof(decoder->timings)is 24 (sixuint32_t), so the check doesn't trip until 24 instead of 6. Should beCOUNT_OF, which the file already uses elsewhere.Both come down to
timings_cntnot being trustworthy: initialize it at alloc so the first reset sees 0, and compare against the element count in the bounds check.Verification
Traced the alloc → reset call path and confirmed
timings_cntis read inreset_state()beforereset()zeroes it, and that RC5'spreamble_markis 0 so it takes that branch. Compiledinfrared_common_decoder.cstandalone against a spec mirroring RC5 and called it the wayinfrared_alloc_decoder()does; before the change AddressSanitizer catches an out-of-bounds access inconsume_samples()from the uninitialized count, and it's clean after.clang-formatis clean, andCOUNT_OFis already included/used in this file.Being upfront on severity: how often the uninitialized read actually bites depends on what byte the heap left in that slot, which I can't observe without flashing. On a reused/dirty heap block it's nonzero and fires; on a fresh-zeroed one it doesn't. The fix is correct either way, and the
sizeof/COUNT_OFcheck is simply wrong regardless. On device this only touches allocation-time init and a bounds constant, so IR read/learn/send for NEC/RC5/RC6/Samsung/etc. should behave exactly as before.Author checklist (Fill this out)
AI usage disclosure (Fill this out):
I used an AI assistant to compile
infrared_common_decoder.cstandalone (outside this repo) with a synthetic RC5-like spec and confirm thattimings_cntis uninitialized after alloc and that ASan catches the resulting out-of-bounds access when reset runs the same wayinfrared_alloc_decoder()calls it. The two-line fix is hand-written.