Bug in ping() causes ESP8266 crash

[jonnygraham]

There's a bug in the ping() that causes a buffer overflow, causing the ESP8266 to crash, if there is data in the buffer (e.g. a message arrives during the ping). readPacket(uint8_t *buffer, uint16_t maxlen,) reads up to the maxlen supplied, unless it's zero. The length checking is as follows:

      buffer[len] = c;
      len++;
      if (len == maxlen) {  // we read all we want, bail

so len is at least 1 so it never reaches maxlen so we can keep writing to buffer well beyond its allocated size.

The call to readPacket with a 0 length is from readFullPacket(). The 'packet length' in a variable called value is 0 (I don't fully understand that code), and then this code runs:

  if (value > (maxsize - (pbuff-buffer) - 1)) {
      DEBUG_PRINTLN(F("Packet too big for buffer"));
      rlen = readPacket(pbuff, (maxsize - (pbuff-buffer) - 1), timeout);
  } else {
    rlen = readPacket(pbuff, value, timeout);
  }

The else is calling readPacket with the value of 0 as max length! Only the ping() seems to create the scenario of value being 0, but I don't know if that's always true or not. I worked around this by adding a check on value being bigger than 0:

  } else if(value > 0) {  // fix: add condition on value>0 because calling readPacket() with a length of zero causes a buffer overflow!
    rlen = readPacket(pbuff, value, timeout);
  }

This prevented the crashes, but I'm not sure if it's the correct fix or if value should never be 0 in the first place.

[ladyada]

hmm! can you do me a favor and turn in DEBUG_PRINT'ing w/the bug still in place so i can see what the packet parser thinks is there?

[brentru]

This issue has been fixed in the latest release, v2.1.0.