Issue with returning zero length string from String.substring()

floathub

New member
Executing the following code will result in a hung Teensy on loop 6:

Code:
String destination_string;
String source_string;
int counter; 

void setup()
{
  source_string = "ABC";
  destination_string = "123";
  counter = 0;
}

void loop()
{
  counter++;
  Serial.print("Beginning loop ");
  Serial.println(counter);
  Serial.send_now();

  Serial.print("  source_string is ");
  Serial.println(source_string);
  Serial.send_now();

  if(counter > 5)
  {
    destination_string = source_string.substring(2,2);
  }
  Serial.print("  destination_string is ");
  Serial.println(destination_string);
  Serial.send_now();

  delay(1000);
}

The issue only occurs if the destination string already contains something (i.e. it is greater than zero length), and only occurs if the substring() call returns a string of zero length. We have tested on Teensy 4.0 and Teensy 4.1, both result in a fairly hard hang (requiring power cycle or button push). Interestingly a set watchdog timer will get it restarted on its own (obviously not included in the code above). Code runs fine and forever on regular Arduino/Mega/etc., just need to comment out the Serial.send_now() calls. This was all tested on Arduino 1.8.13, with Teensy 1.5.3 (on Linux).

We have started to look at the Teensy String code, but it will probably take us a while to muddle through that. Let us know if we can provide any further details on this that might be helpful.
 
I can confirm this - but it doesn't happen on the T3.2 despite the printing and String
code seeming identical where it should matter.
 
At first glance, the culprit seems to be in String::move, which is the function that ends up being called by destination_string = source_string.substring(2,2);. In this case, the lhs has an available buffer, and it's capacity (3) is greater than or equal to the length (0) of the rhs, so the code tries to do a strcpy. Unfortunately, the value of rhs.buffer is NULL.

Code:
void String::move(String &rhs)
{
   if (buffer) {
      if (capacity >= rhs.len) {
         strcpy(buffer, rhs.buffer);
         len = rhs.len;
         rhs.len = 0;
         return;
      } else {
         free(buffer);
      }
   }
   buffer = rhs.buffer;
   capacity = rhs.capacity;
   len = rhs.len;
   rhs.buffer = NULL;
   rhs.capacity = 0;
   rhs.len = 0;
}

My initial hunch is that adding a check for a non NULL source buffer might do the trick, but I'd like to hear what others think.

Code:
void String::move(String &rhs)
{
   if (buffer) {
      if (capacity >= rhs.len [COLOR="#FF0000"]&& rhs.buffer[/COLOR]) {
         strcpy(buffer, rhs.buffer);
         len = rhs.len;
         rhs.len = 0;
         return;
      } else {
         free(buffer);
      }
   }
   buffer = rhs.buffer;
   capacity = rhs.capacity;
   len = rhs.len;
   rhs.buffer = NULL;
   rhs.capacity = 0;
   rhs.len = 0;
}
 
Perhaps:
Code:
void String::move(String &rhs)
{
	if (buffer) {
		if (capacity >= rhs.len) {
			if (rhs.buffer)
				strcpy(buffer, rhs.buffer);
			else
				*buffer = '\0';
			len = rhs.len;
			rhs.len = 0;
			return;
		} else {
			free(buffer);
		}
	}
	buffer = rhs.buffer;
	capacity = rhs.capacity;
	len = rhs.len;
	rhs.buffer = NULL;
	rhs.capacity = 0;
	rhs.len = 0;
}

These String classes really need a high coverage testsuide, there have been several nasty bugs over the years
 
I submitted a bug to the Arduino bug report system about this, WString seems to have been in error like
this for a while, but no-one noticed as most microcontrollers let you deference NULL without faulting!
https://github.com/arduino/Arduino/issues/11547

[ BTW I noticed the WString version of substring sorts the start/end arguments which is pretty unusual (and probably
a nasty gotcha in fact).
 
[ BTW I noticed the WString version of substring sorts the start/end arguments which is pretty unusual (and probably
a nasty gotcha in fact).

We had noticed this argument sorting as well, while testing cases to report the above issue. It does seem odd that substring(4, 1) of "foobar" returns "oob".
 
Rather than '' or 'boo' which could actually be handy in some circumstances. High level languages with exception
handling normally fix all these awkward corners by throwing an exception.
 
Back
Top