From 0b19d9437ddfd6d728d9d15de8864a9c56ce92b7 Mon Sep 17 00:00:00 2001
From: David Conran <crankyoldgit@users.noreply.github.com>
Date: Tue, 6 Jul 2021 13:47:10 +1000
Subject: [PATCH] [BUG] Illegal Heap write in rawbuf when the capture has
 overflowed. (#1517)

* Fix an issue where we write past the end of the capture buffer when it is full. Two options to fix this:
  1. Extend all capture buffers by 1 entry. i.e. upto 4 bytes of extra unused heap and some FLASH/PROGMEM bytes. _or_
  2. Skip the memory write when we have overflowed. i.e. Possibly slightly more than 4 bytes of FLASH/PROGMEM used.
  - CPU overhead should be about the same.
  - Given heap & memory is a more critical resource than Flash/PROGMEM, opting for Option 2.
* Add a helper method `IRrecv::_getParamsPtr` to access `params` in Unit tests.
* Unit tests so we can be sure it is fixed, and it doesn't happen again.

Kudos to @davepl for reporting the issue and diagnosing the offending line of code.

Fixes #1516
---
 src/IRrecv.cpp       | 11 ++++++++++-
 src/IRrecv.h         |  3 +++
 test/IRrecv_test.cpp | 43 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/src/IRrecv.cpp b/src/IRrecv.cpp
index 4a6281b45..38e9e5ec0 100644
--- a/src/IRrecv.cpp
+++ b/src/IRrecv.cpp
@@ -518,7 +518,9 @@ bool IRrecv::decode(decode_results *results, irparams_t *save,
   // interrupt. decode() is not stored in ICACHE_RAM.
   // Another better option would be to zero the entire irparams.rawbuf[] on
   // resume() but that is a much more expensive operation compare to this.
-  params.rawbuf[params.rawlen] = 0;
+  // However, don't do this if rawbuf is already full as we stomp over the heap.
+  // See: https://github.com/crankyoldgit/IRremoteESP8266/issues/1516
+  if (!params.overflow) params.rawbuf[params.rawlen] = 0;
 
   bool resumed = false;  // Flag indicating if we have resumed.
 
@@ -1882,4 +1884,11 @@ uint16_t IRrecv::matchManchesterData(volatile const uint16_t *data_ptr,
   *result_ptr = GETBITS64(data, 0, nbits);
   return offset;
 }
+
+#if UNIT_TEST
+/// Unit test helper to get access to the params structure.
+volatile irparams_t *IRrecv::_getParamsPtr(void) {
+  return &params;
+}
+#endif  // UNIT_TEST
 // End of IRrecv class -------------------
diff --git a/src/IRrecv.h b/src/IRrecv.h
index ed204cbb0..bdcca0cc0 100644
--- a/src/IRrecv.h
+++ b/src/IRrecv.h
@@ -162,6 +162,9 @@ class IRrecv {
 #if DECODE_HASH
   uint16_t _unknown_threshold;
 #endif
+#ifdef UNIT_TEST
+  volatile irparams_t *_getParamsPtr(void);
+#endif  // UNIT_TEST
   // These are called by decode
   uint8_t _validTolerance(const uint8_t percentage);
   void copyIrParams(volatile irparams_t *src, irparams_t *dst);
diff --git a/test/IRrecv_test.cpp b/test/IRrecv_test.cpp
index 76a0d51d8..2d32847d3 100644
--- a/test/IRrecv_test.cpp
+++ b/test/IRrecv_test.cpp
@@ -42,6 +42,49 @@ TEST(TestIRrecv, IRrecvDestructor) {
   delete irrecv_ptr;
 }
 
+TEST(TestIRrecv, DecodeHeapOverflow) {
+  // Check that we handle the rawbuf correctly when we fill it. e.g. overflow.
+  // Ref: https://github.com/crankyoldgit/IRremoteESP8266/issues/1516
+  IRrecv irrecv(1);
+  irrecv.enableIRIn();
+  ASSERT_EQ(kRawBuf, irrecv.getBufSize());
+  volatile irparams_t *params_ptr = irrecv._getParamsPtr();
+  // replace the buffer with a slightly bigger one to see if we go past the end
+  // accidentally.
+  params_ptr->rawbuf = new uint16_t[kRawBuf + 10];
+  ASSERT_EQ(kRawBuf, irrecv.getBufSize());  // Should not change.
+  // Fill the raw buffer with canaries
+  //  Values of 100 for the proper buffer size, & values of 99 for the extras
+  for (uint16_t i = 0; i < irrecv.getBufSize() + 10; i++) {
+    params_ptr->rawbuf[i] = 100;
+    if (i >= irrecv.getBufSize()) params_ptr->rawbuf[i]--;
+  }
+  ASSERT_EQ(100, params_ptr->rawbuf[kRawBuf - 1]);
+  EXPECT_EQ(99, params_ptr->rawbuf[kRawBuf]);
+  EXPECT_EQ(99, params_ptr->rawbuf[kRawBuf + 1]);
+  ASSERT_EQ(kRawBuf, params_ptr->bufsize);
+  decode_results results;
+  // Mock up the rest of params like we've received a message that has used
+  // all the rawbuf.
+  params_ptr->rawlen = kRawBuf;
+  params_ptr->overflow = true;
+  params_ptr->rcvstate = kStopState;
+  // Need to tweak results structure too.
+  results.rawbuf = params_ptr->rawbuf;
+  results.rawlen = params_ptr->rawlen;
+  results.overflow = params_ptr->overflow;
+
+  // Do the decode.
+  ASSERT_TRUE(irrecv.decode(&results));
+  // Yay, nothing exploded! Now check everything is as we expect
+  // w.r.t. the buffer.
+  ASSERT_EQ(kRawBuf, params_ptr->rawlen);
+  ASSERT_TRUE(params_ptr->overflow);
+  ASSERT_EQ(100, params_ptr->rawbuf[params_ptr->rawlen - 1]);
+  EXPECT_EQ(99, params_ptr->rawbuf[params_ptr->rawlen]);
+  EXPECT_EQ(99, params_ptr->rawbuf[params_ptr->rawlen + 1]);
+}
+
 // Tests for copyIrParams()
 
 TEST(TestCopyIrParams, CopyEmpty) {