Stack smashing protection failure occurs in String

[Hieromon]

Hardware:

Board: ESP32 Dev Module, also Heltec WiFi kit 32
Core Installation version: 1.0.3-rc2
IDE name: Arduino IDE 1.8.8 also 1.8.9
Flash Frequency: 80MHz
PSRAM enabled: no
Upload Speed: 921600
Computer OS: Windows 7, Windows 10

Description:

Stack smashing protection failure occurs in String. This problem seems to be related to the order of dynamic allocation for String data in the function and the length of the string.

Sketch:

#include <Arduino.h>

void setup() {
  delay(1000);
  Serial.begin(115200);
  String  reps = "/";
  String  replacement = "&#47;";
  String  source = "/save";
  Serial.printf("source:%s(%d)\nreplacement:%s(%d)\nreps:%s(%d)\n", source.c_str(), source.length(), replacement.c_str(), replacement.length(), reps.c_str(), reps.length());
  Serial.printf("source#L11:%s(%d)\n", source.c_str(), source.length());
  source.replace(reps, replacement);
  Serial.printf("source#L13:%s(%d)\n", source.c_str(), source.length());
  source.trim();
  Serial.printf("source#L15:%s(%d)\n", source.c_str(), source.length());
}

void loop() {
}

Debug Messages:

source:/save(5)
replacement:/(5)
reps:/(1)
source#L11:/save(5)
source#L13:/save(9)
source#L15:/save(9)

Stack smashing protect failure!

abort() was called at PC 0x400d5b70 on core 1

Backtrace: 0x4008b544:0x3ffb1ee0 0x4008b771:0x3ffb1f00 0x400d5b70:0x3ffb1f20 0x400d0d88:0x3ffb1f40 0x400d2027:0x3ffb1fb0 0x4008a091:0x3ffb1fd0

Rebooting...
ets Jun  8 2016 00:22:57

rst:0xc (SW_CPU_RESET),boot:0x17 (SPI_FAST_FLASH_BOOT)
configsip: 0, SPIWP:0xee
clk_drv:0x00,q_drv:0x00,d_drv:0x00,cs0_drv:0x00,hd_drv:0x00,wp_drv:0x00
mode:DIO, clock div:1
load:0x3fff0018,len:4
load:0x3fff001c,len:928
ho 0 tail 12 room 4
load:0x40078000,len:8740
load:0x40080400,len:5816
entry 0x4008069c

Stack dump decoded:

Decoding 7 results
0x400d5de4: __stack_chk_fail at /Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/esp32/stack_check.c line 36
0x4008b544: invoke_abort at /Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/esp32/panic.c line 707
0x4008b771: abort at /Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/esp32/panic.c line 707
0x400d5de4: __stack_chk_fail at /Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/esp32/stack_check.c line 36
0x400d0de0: setup() at C:\Users\hieromon\Documents\Arduino\StrRep/StrRep.ino line 17
0x400d21a7: loopTask(void*) at C:\Users\hieromon\AppData\Local\Arduino15\packages\esp32\hardware\esp32\1.0.3-rc2\cores\esp32/main.cpp line 14
0x4008a091: vPortTaskWrapper at /Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/freertos/port.c line 355 (discriminator 1)

Preliminary survey:

1. The violation seems to be caused by releasing the string buffer in the termination process of the setup().

2. Oddly if the position of the reps initialization statement moves, the problem does not occur.

#include <Arduino.h>

void setup() {
  delay(1000);
  Serial.begin(115200);
  String  replacement = "&#47;";
  String  source = "/save";
  String  reps = "/"; // move to here
  Serial.printf("source:%s(%d)\nreplacement:%s(%d)\nreps:%s(%d)\n", source.c_str(), source.length(), replacement.c_str(), replacement.length(), reps.c_str(), reps.length());
  // if (source.length() == reps.length() + replacement.length())
    // source += " ";
  Serial.printf("source#L11:%s(%d)\n", source.c_str(), source.length());
  source.replace(reps, replacement);
  Serial.printf("source#L13:%s(%d)\n", source.c_str(), source.length());
  source.trim();
  Serial.printf("source#L15:%s(%d)\n", source.c_str(), source.length());
}

void loop() {
}

The above sketch successfully ended.

[lbernstone]

It seems to be something unsafe in String::replace. If you convert them to char* with source.replace(reps.c_str(), replacement.c_str()); it prevents any modification of the strings.

[atanisoft]

@Hieromon can you test this usecase with 1.0.2 ? I'm wondering if this commit is causing the issue

[lbernstone]

I bisected to commit ab309e4
@earlephilhower do you get notifications here? Issue is 100% reproducible.

[earlephilhower]

@lbernstone unfortunately that's my big "convert to SSO" commit which doesn't really narrow it down. :(

I do most of my work on the 8266 where there's no such protection, but we do run on valgrind on the host tests (imperfect match for the 8266 HW, but best we can do).

We have had several fixes to the SSO implementation, involving string replacement:
https://github.com/esp8266/Arduino/commits/master/cores/esp8266/WString.cpp

In particular, esp8266/Arduino@54240d2#diff-8d9e71e16d437343017df828f0528f63

A reproducible failing test case was found there and corrected, this could be the same thing.

[Hieromon]

@atanisoft I tested with 1.0.2 and it works fine.
@earlephilhower Is the same issue I have pointed out in the past with esp8266? esp8266/Arduino#6192

[atanisoft]

@earlephilhower perhaps a resync should be done?

[earlephilhower]

Yes, @Hieromon , it looks like the same thing.

@atanisoft I'll put in a PR later this morning.

[earlephilhower]

@Hieromon, could you please check the PR linked above and let us know if that cleans up your error? That PR is definitely necessary for the -32, no matter what.

If there's still a problem then I've got to repro on the host and make a new PR in both the 8266 and 32 repos.

[Hieromon]

@earlephilhower Your PR works fine, it's a great job.
I tested with my library as AutoConnect too and think that it is safe to merge this PR.

Thank you for the quick response as always.
Issue closed due to fixed.

[earlephilhower]

Might want to reopen until it's merged, @Hieromon ? The merge will close it automatically when completed.

[Hieromon]

@earlephilhower Ok, My test is roughly finished, but let's resume it just in case and close by merging.