Bug in String::copy, out of bounds crash

[TD-er]

Basic Infos

• This issue complies with the issue POLICY doc.
• I have read the documentation at readthedocs and the issue is not addressed there.
• I have tested that the issue is present in current master branch (aka latest git).
• I have searched the issue tracker for a similar issue.
• If there is a stack dump, I have decoded it.
• I have filled out all fields below.

Problem Description

See line 291 (and line 280):

Arduino/cores/esp8266/WString.cpp

Lines 284 to 293 in eda4e08

	String &String::copy(const __FlashStringHelper *pstr, unsigned int length) {
	if (!reserve(length)) {
	invalidate();
	return *this;
	}
	setLen(length);
	memcpy_P(wbuffer(), (PGM_P)pstr, length); // We know wbuffer() cannot ever be in PROGMEM, so memcpy safe here
	wbuffer()[length] = 0;
	return *this;
	}

When allocating an array of N elements, accessing element [N] is out of bounds.

This only happens sometimes at very specific string lengths, as setLen only allocates in multiples of N bytes.

N.B. similar issue for ESP32, so I will also add an issue there.

Suggested fix:

reserve(length + 1);

N.B. This function is likely to be called with strlen() or strlen_P() as argument, which does not include the ending null character.

[TD-er]

There are other functions which start to look 'odd' to me, but since they are there for so long, I start doubting myself if I'm seeing bugs everywhere....

For example:

Arduino/cores/esp8266/WString.cpp

Lines 209 to 227 in eda4e08

	bool String::changeBuffer(unsigned int maxStrLen) {
	// Can we use SSO here to avoid allocation?
	if (maxStrLen < sizeof(sso.buff) - 1) {
	if (isSSO() || !buffer()) {
	// Already using SSO, nothing to do
	uint16_t oldLen = len();
	setSSO(true);
	setLen(oldLen);
	} else { // if bufptr && !isSSO()
	// Using bufptr, need to shrink into sso.buff
	const char *temp = buffer();
	uint16_t oldLen = len();
	setSSO(true);
	setLen(oldLen);
	memcpy(wbuffer(), temp, maxStrLen);
	free((void *)temp);
	}
	return true;
	}

Line 222
I think setLen should be set to the minimum of oldLen and maxStrLen

And this one:

Arduino/cores/esp8266/WString.cpp

Lines 343 to 361 in eda4e08

	bool String::concat(const String &s) {
	// Special case if we're concatting ourself (s += s;) since we may end up
	// realloc'ing the buffer and moving s.buffer in the method called
	if (&s == this) {
	unsigned int newlen = 2 * len();
	if (!s.buffer())
	return false;
	if (s.len() == 0)
	return true;
	if (!reserve(newlen))
	return false;
	memmove_P(wbuffer() + len(), buffer(), len());
	setLen(newlen);
	wbuffer()[newlen] = 0;
	return true;
	} else {
	return concat(s.buffer(), s.len());
	}
	}

Shouldn't reserve be called like this: reserve(newlen + 1) ?
The same for:

• bool String::concat(const char *cstr, unsigned int length)
• bool String::concat(const __FlashStringHelper *str)
• String operator +(const String &lhs, String &&rhs)
• String operator +(char lhs, const String &rhs)
• String operator +(const char *lhs, const String &rhs)

Or maybe the simplest fix might be to adapt String::changeBuffer:

size_t newSize = (maxStrLen + 16 + 1) & (~0xf);

[mcspr]

reserve(length + 1);

I'd suggest not, internals care about string length not internal representation aka cstring with null byte.

Copies mentioning null should probably be considered a bug, delegating null byte handling to length setter. Meaning, we never try to do anything with it outside of length setter

This only happens sometimes at very specific string lengths

...such as?

[TD-er]

I have not yet been able to log where it crashes, so I don't know exactly which string it crashes on.

And I also did some testing here to see if my assumption about this is correct:

size_t newSize = (maxStrLen + 16) & (~0xf);

This always returns a newSize which is at least 1 larger than the requested size.

So I think this should be put on hold till I found the true crash reason.

Right now it seems like it might be a combination of a String copy where the string size has shrunk due to a few replace calls.

It is in the assignment of a String to a std::map<uin16_t, String>
But the really strange part is that the top of the stack seems to call String::copy(__FlashStringHelper...):

0x40286fd8 in String::copy(__FlashStringHelper const*, unsigned int) at ??:?
0x402e4437 in chip_v6_unset_chanfreq at ??:?
0x402392b9 in std::map<unsigned short, String, std::less<unsigned short>, std::allocator<std::pair<unsigned short const, String> > >::operator[](unsigned short const&) at ??:?
0x4010162a in malloc at ??:?
0x40101343 in umm_free_core at umm_malloc.cpp:?
0x40288354 in operator new(unsigned int) at ??:?
0x402392b9 in std::map<unsigned short, String, std::less<unsigned short>, std::allocator<std::pair<unsigned short const, String> > >::operator[](unsigned short const&) at ??:?
0x40286d79 in String::invalidate() at ??:?

[mcspr]

Can you point to the specific code & full stack dump w/ exc address etc.? My understanding this is related to the letscontrolit/ESPEasy#5013, but the original issue has neither

[TD-er]

This is while running on my PC, so probably not the same addresses.

0x40286fd8 in String::copy(__FlashStringHelper const*, unsigned int) at ??:?
0x402e4437 in chip_v6_unset_chanfreq at ??:?
0x402392b9 in std::map<unsigned short, String, std::less<unsigned short>, std::allocator<std::pair<unsigned short const, String> > >::operator[](unsigned short const&) at ??:?
0x4010162a in malloc at ??:?
0x40101343 in umm_free_core at umm_malloc.cpp:?
0x40288354 in operator new(unsigned int) at ??:?
0x402392b9 in std::map<unsigned short, String, std::less<unsigned short>, std::allocator<std::pair<unsigned short const, String> > >::operator[](unsigned short const&) at ??:?
0x40286d79 in String::invalidate() at ??:?
0x402393c1 in UserVarStruct::getPreprocessedFormula(unsigned char, unsigned short) const at ??:?
0x40286d98 in String::~String() at ??:?
0x4023b0aa in TaskValues_Data_t::getAsString(unsigned char, Sensor_VType, unsigned char) const at ??:?
0x4023976b in UserVarStruct::applyFormula(unsigned char, unsigned short, String const&, Sensor_VType, bool) const at ??:?
0x40286f5c in String::copy(char const*, unsigned int) at ??:?
0x40239aa4 in UserVarStruct::getAsString(unsigned char, unsigned short, Sensor_VType, unsigned char, bool) const at ??:?
0x40239870 in UserVarStruct::getRawOrComputed(unsigned char, unsigned short, Sensor_VType, bool) const at ??:?
0x4028704a in String::move(String&) at ??:?
0x40239a71 in UserVarStruct::getAsString(unsigned char, unsigned short, Sensor_VType, unsigned char, bool) const at ??:?
0x40250d4d in doFormatUserVar(EventStruct*, unsigned char, bool, bool&) at ??:?
0x4028704a in String::move(String&) at ??:?
0x40287000 in String::move(String&) at ??:?
0x4010162a in malloc at ??:?
0x40286f7f in String::String(char const*) at ??:?
0x40250dc4 in formatUserVarNoCheck(unsigned char, unsigned char) at ??:?
0x40286e22 in String::changeBuffer(unsigned int) at ??:?
0x40266fcf in P026_data_struct::Plugin_Read(EventStruct*) at ??:?
0x402142ac in Plugin_026(unsigned char, EventStruct*, String&) at ??:?
0x40214425 in Plugin_026(unsigned char, EventStruct*, String&) at ??:?
0x4025515e in PluginCall(deviceIndex_t, unsigned char, EventStruct*, String&) at ??:?
0x40287148 in String::operator=(String const&) at ??:?
0x40243ea8 in prepare_I2C_by_taskIndex(unsigned char, deviceIndex_t) at ??:?
0x402447f0 in PluginCall(unsigned char, EventStruct*, String&) at ??:?
0x40243d3c in getDeviceIndex_from_TaskIndex(unsigned char) at ??:?
0x40286d79 in String::invalidate() at ??:?
0x40286d98 in String::~String() at ??:?
0x4022bcbc in checkDeviceVTypeForTask(EventStruct*) at ??:?
0x4023dd34 in SensorSendTask(EventStruct*, unsigned long, unsigned long) at ??:?
0x40233521 in EventStruct::EventStruct(unsigned char) at ??:?
0x4024cb7a in ESPEasy_Scheduler::process_task_device_timer(SchedulerTimerID, unsigned long) at ??:?
0x40101300 in umm_free_core at umm_malloc.cpp:?
0x40100b00 in stopWaveform at ??:?
0x40288610 in esp_yield at ??:?
0x40261cb9 in ESPEasy_Scheduler::handle_schedule() at ??:?
0x40295bc8 in spiffs_cache_page_get_by_fd at ??:?
0x40295bc8 in spiffs_cache_page_get_by_fd at ??:?
0x40240272 in WiFiConnected() at ??:?
0x4024350b in ESPEasy_loop() at ??:?
0x4020e488 in loop at ??:?
0x40288737 in loop_wrapper() at core_esp8266_main.cpp:?
0x401004f9 in cont_wrapper at ??:?

My current work-around (stacktrace is not using this work-around as it doesn't crash anymore) is to not to use the _preprocessedFormula[key] = RulesCalculate_t::preProces(Cache.getTaskDeviceFormula(taskIndex, varNr));, but instead use _preprocessedFormula.emplace(std::make_pair(key, RulesCalculate_t::preProces(Cache.getTaskDeviceFormula(taskIndex, varNr)));

I am looking into the String code to see if I can come up with some way to have either a String with a length longer than what is allocated, or a 0-character inbetween.

[mcspr]

I am looking into the String code to see if I can come up with some way to have either a String with a length longer than what is allocated, or a 0-character inbetween.

If '\0' is embedded somewhere, there is an issue with replace() doing strlen() which probably does not do the right thing. Same with anything constructing / assigning using plain pointer

[TD-er]

String::copy(__FlashStringHelper const*, unsigned int) (and the const char* version) is used when simply copying the String. This is done when assigning to the map using [key] = ....
So if the length is more than the allocated, then it will crash.
I think it might be possible to have some String object which will then be calling String::changeBuffer with a size smaller than the sso.buf size.

In my test using ESPEasy, I do have a string which was 13 bytes and is reduced to some length less than 11 bytes.

The reason I'm also thinking about a \0 inbetween is because the MQTT connection gets lost (I have not been able to reproduce this myself), however it is also possible to get this result when length is used to determine the message to be published to MQTT.

Anyway, I don't have conclusive example code yet and will try to get some sleep first as it has been a very busy day today.