String::substring deferences NULL if the returned value is the empty string. This is due to a bug in String::move

[MarkTillotson]

In certain situations using the result of the String::substring() method will (read) deference NULL.
On some architectures this crashes the processor (Notable Teensy4)

[ using Arduino 1.8.13 / Teensyduino 1.53 - but the code in WString.cpp doesn't seem to have changed
for a long time ]

Necessary conditions:
substring() returns a null String
an existing non-null String is overwritten by this result.

The fault is that the String::move method in these circumstances calls strcpy with the source argument
as NULL.

You can see the bug report on the Teensy forum here: https://forum.pjrc.com/threads/67275-Issue-with-returning-zero-length-string-from-String-substring()

Example code to reproduce on Teensy4 (and presumably other architectures where address 0 faults if
deferenced):

void setup() 
{
  Serial.begin(115200) ;
  delay(500) ;
  Serial.println ("Testing:") ;
  String a = "ABCDEF" ;
  Serial.print ("1: '") ; Serial.print (a) ;
  Serial.println ("'") ;
  String b = a.substring(2, 2) ;
  Serial.print ("2: '") ; Serial.print (b) ;
  Serial.println ("'") ;
  b = a.substring(2, 2) ;
  Serial.print ("3: '") ; Serial.print (b) ;
  Serial.println ("'") ; delay(10) ;
  b = "1234" ;
  b = a.substring(2, 2) ;
  Serial.print ("4: '") ; Serial.print (b) ;
  Serial.println ("'") ;
  
}

void loop() {}

Expected result:

Testing:
1: 'ABCDEF'
2: ''
3: ''
4: ''

What happens instead:

Testing:
1: 'ABCDEF'
2: ''
3: ''

and the system hangs/crashes.

The definition of String::move I have:

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;
}

Suggested fix:

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;
}

[matthijskooijman]

Thanks or your report!

Development of this code happens in https://github.com/arduino/ArduinoCore-API now, so I think issue can be moved there. @per1234, I see you also looked at this issue, did you decide not to move it for some reason?

@MarkTillotson, there is already a pending PR to fix the move constructor to really do a move, see #21. I suspect that that might actually fix your issue as well, could you have a look at that to check?

[per1234]

@matthijskooijman I agree that it should be moved to arduino/ArduinoCore-API. Unfortunately, I don't have write permissions in any of the arduino/ArduinoCore* repositories and GitHub only allows transfering issues between repositories when you have write permissions in both of them. If anyone is able to make the transfer, I would be grateful.

[matthijskooijman]

Unfortunately, I don't have write permissions in any of the arduino/ArduinoCore* repositories

Ah, that explains. Apparently I do, so I moved the issue :-)

[MarkTillotson]

@MarkTillotson, there is already a pending PR to fix the move constructor to really do a move, see #21. I suspect that that might actually fix your issue as well, could you have a look at that to check?

Yes, that both fixes it and the code for move() looks much cleaner. I think this report can be marked as addressed by that PR...

[matthijskooijman]

Thanks for confirming, I've marked this issue as fixed by the PR.