Trashage of the heap

[davepl]

In Irrecv.cpp, you poke values into rawbuf[rawlen] in many cases. However, you only allocate 100 bytes. When rawlen gets to be 100, you still do it. So you actually need to allocate one more byte than you do (or stop writing past the end of the buffer, but its easier to allocate an extra byte).

I've confirmed this change locally stops it from overwriting the heap tag.

Thanks!

To fix this, in IRRecv::IRecv, change

params.rawbuf = new uint16_t[bufsize];

to

params.rawbuf = new uint16_t[bufsize+1];

[crankyoldgit]

Thanks for the report, I'll look it to it soon.

[crankyoldgit]

Hmm. I'm not sure you're correct, but I'll look at writing some unit tests to confirm/verify that if you disagree or have proof.

The code that adds to the array/memory and stops the capture process is:

IRremoteESP8266/src/IRrecv.cpp

Lines 186 to 204 in 3c1862f

	uint16_t rawlen = params.rawlen;
	if (rawlen >= params.bufsize) {
	params.overflow = true;
	params.rcvstate = kStopState;
	}
	if (params.rcvstate == kStopState) return;
	if (params.rcvstate == kIdleState) {
	params.rcvstate = kMarkState;
	params.rawbuf[rawlen] = 1;
	} else {
	if (now < start)
	params.rawbuf[rawlen] = (UINT32_MAX - start + now) / kRawTick;
	else
	params.rawbuf[rawlen] = (now - start) / kRawTick;
	}
	params.rawlen++;

Let's assume params.bufsize is 100 per your example, and params.rawlen is 98. i.e. we are nearing the end of the buffer, but not there yet, and we are still capturing data (i.e. we haven't timed out), thus params.rcvstate is kMarkState.
We both agree that params.rawbuf[99] is the last entry (a uint16_t) of the array, beyond that we are stomping over memory.

Okay, stepping through the code:
We're at 98 ... and we get a interrupt for new data, thus execute gpio_intr() (which contains that above code snippet).

1. 98 is less than bufsize (100) so we skip the if (rawlen >= params.bufsize) {} clause.
2. The rcvstate is kMarkState so we skip the (params.rcvstate == kStopState) {} clause.
3. Next, if (params.rcvstate == kIdleState) fails, so we take the else path. This path puts a value in params.rawbuf[rawlen] i.e. params.rawbuf[98] which is fine/safe.
4. Next, we rawlen++, so rawlen is now 99.
5. We basically exit the routine as we've now handled that interrupt.

Now we're at 99 ... and we get another interrupt for new data, thus execute gpio_intr() again.

1. 99 is less than bufsize (100) so we skip the if (rawlen >= params.bufsize) {} clause.
2. The rcvstate is kMarkState so we skip the (params.rcvstate == kStopState) {} clause.
3. Next, if (params.rcvstate == kIdleState) fails, so we take the else path. This path puts a value in params.rawbuf[rawlen] i.e. params.rawbuf[99] which is fine/safe, and is the last safe entry in the array etc.
4. Next, we rawlen++, so rawlen is now 100.
5. We basically exit the routine as we've now handled that interrupt.

Now we're at 100, the buffer is technically already full ... and we get another interrupt for new data, thus execute gpio_intr() again.

1. 100 is equal to bufsize (100) so we execute the if (rawlen >= params.bufsize) {} clause this time. i.e. Set an overflow flag, and rcvstate is set to kStopState;
2. The rcvstate is now kStopState so we execute the (params.rcvstate == kStopState) {} clause. i.e. return.
   There is no further writing to rawbuf or incrementing rawlen. It's locked at 100 (i.e. bufsize)

So, the end result of filling the buffer is rawlen gets the value of bufsize, but the last write to rawbuf is rawbuf[rawlen - 1] when using the final value of rawlen. i.e. In this case rawbuf[100 - 1] or rawbuf[99], which is safe for a 100 entry array.

Am I missing something here?

[davepl]

Not sure where you’re looking, but here’s the offending code: // i.e. Stopped collecting IR data. // It's junk as we never wrote an entry to it and can only confuse decoding. // This is done here rather than logically the best place in read_timeout() // as it saves a few bytes of ICACHE_RAM as that routine is bound to an // 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; When rawlen == sizeof(rawbuf), boom. So I’m assuming I call handle, it must wind up leaving rawlen at 100, and when decode is called, it immediately stomps the buffer at 100 without checking, Are you writing this zero tag to the END of the data? If so, you need to allocate that extra byte, which I don’t think you are. Remember I’m just trying to help because I like the library… adding the byte to the allocation fixes it entirely for me, but it’s not the right fix, and others will hit this. Thanks Dave PS: In you gpio_intr seem to add one to rawbuf length. Could this be part of the issue? Is that increment included in the allocation such that you’re ready for the longest “rawlen + 1” in this case?

…

On Jul 4, 2021, at 8:37 PM, David Conran ***@***.***> wrote: Hmm. I'm not sure you're correct, but I'll look at writing some unit tests to confirm/verify that if you disagree or have proof. The code that adds to the array/memory and stops the capture process is: https://github.com/crankyoldgit/IRremoteESP8266/blob/3c1862f1ad1ad4f92fbe56a370c269d5553ca275/src/IRrecv.cpp#L186-L204 <https://github.com/crankyoldgit/IRremoteESP8266/blob/3c1862f1ad1ad4f92fbe56a370c269d5553ca275/src/IRrecv.cpp#L186-L204> Let's assume params.bufsize is 100 per your example, and params.rawlen is 98. i.e. we are nearing the end of the buffer, but not there yet, and we are sill capturing data (i.e. we haven't timed out), thus params.rcvstate is kMarkState. We both agree that params.rawbuf[99] is the last byte of the array, beyond that we are stomping over memory. Okay, stepping through the code: We're at 98 ... and we get a interrupt for new data, thus execute gpio_intr() (which contains that above code snippet). 98 is less than bufsize (100) so we skip the if (rawlen >= params.bufsize) {} clause. The rcvstate is kMarkState so we skip the (params.rcvstate == kStopState) {} clause. Next, if (params.rcvstate == kIdleState) fails, so we take the else path. This path puts a value in params.rawbuf[rawlen] i.e. params.rawbuf[98] which is fine/safe. Next, we rawlen++, so rawlen is now 99. We basically exit the routine as we've now handled that interrupt. Now we're at 99 ... and we get another interrupt for new data, thus execute gpio_intr() again. 99 is less than bufsize (100) so we skip the if (rawlen >= params.bufsize) {} clause. The rcvstate is kMarkState so we skip the (params.rcvstate == kStopState) {} clause. Next, if (params.rcvstate == kIdleState) fails, so we take the else path. This path puts a value in params.rawbuf[rawlen] i.e. params.rawbuf[99] which is fine/safe, and is the last safe entry in the array etc. Next, we rawlen++, so rawlen is now 100. We basically exit the routine as we've now handled that interrupt. Now we're at 100, the buffer is technically already full ... and we get another interrupt for new data, thus execute gpio_intr() again. 100 is equal to bufsize (100) so we execute the if (rawlen >= params.bufsize) {} clause this time. i.e. Set an overflow flag, and rcvstate is set to kStopState; The rcvstate is now kStopState so we execute the (params.rcvstate == kStopState) {} clause. i.e. return. There is no further writing to rawbuf or incrementing rawlen. It's locked at 100 (i.e. bufsize) So, the end result of filling the buffer is rawlen gets the value of bufsize, but the last write to rawbuf is rawbuf[rawlen - 1] when using the final value of rawlen. i.e. In this case rawbuf[100 - 1] or rawbuf[99], which is safe for a 100 entry array. Am I missing something here? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1516 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCF4AVP6BNXZZI473MN3TWESIBANCNFSM47ZVK5OA>.

[crankyoldgit]

Yep. I see it now. I forgot that I had moved some of the code out of the interrupt routines into other places, to reduce IRAM usaged. i.e. into IRrecv::decode(). Thus I didn't even think to check decode() because nothing other than the interrupt routines, IRrecv::copyIrParams(), & IRrecv::crudeNoiseFilter() should write to a rawbuf. Fix incoming. Thanks for the second set of eyes.

[davepl]

No problem, glad you found it! Cheers Dave

…

On Jul 5, 2021, at 7:13 AM, David Conran ***@***.***> wrote: Yep. I see it now. I forgot that I had moved some of the code out of the interrupt routines into other places, to reduce IRAM usaged. i.e. into IRrecv::decode(). Thus I didn't even think to check decode() because nothing other than the interrupt routines, IRrecv::copyIrParams(), & IRrecv::crudeNoiseFilter()should write to arawbuf`. Fix incoming. Thanks for the second set of eyes. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1516 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCF6KCIUHSXOL2B6LHBLTWG42DANCNFSM47ZVK5OA>.

[crankyoldgit]

@davepl Please checkout/try branch https://github.com/crankyoldgit/IRremoteESP8266/tree/Issue1516 and let me know if it works for you?
See 2988443 for info on the coding decision. Your suggested solution is probably safer in the balance of things. i.e. in case there is another similar situation. However the pressure on usage of memory vs Flash leans me to the path I've taken. However, I'm really on the fence. If it wasn't on an imbedded system, I'd do both. ;-)

Now to write some tests so this never happens again.... sigh

[crankyoldgit]

And now with unit tests confirming that this particular issue in decode() has been squashed. Phew.

[crankyoldgit]

FYI, the committed & merged changes have been included in the newly released version of the library. i.e. v2.7.19