SNTP: configTime server cannot reference temporary char pointer

[mcspr]

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.

Platform

• Hardware: ESP8285
• Core Version: 2.6.3 / [63b73bf]
• Development Env: PlatformIO
• Operating System: Linux

Settings in IDE

• Defaults

Problem Description

With this piece of code below:

Arduino/cores/esp8266/time.cpp

Line 61 in 63b73bf

sntp_setservername(id, (char*) name_or_ip);

If user does something like:

const String server = retrieveMyServerSettings();
configTime("UTC0", server.c_str());

Because lwip only stores a pointer
https://git.savannah.nongnu.org/cgit/lwip.git/tree/src/apps/sntp/sntp.c?h=STABLE-2_1_x#n876

void
sntp_setservername(u8_t idx, const char *server)
{
  LWIP_ASSERT_CORE_LOCKED();
  if (idx < SNTP_MAX_SERVERS) {
    sntp_servers[idx].name = server;
  }
}

Pointer becomes garbage after some time.

Library example is not affected

MCVE Sketch

#include <ESP8266WiFi.h>
#include <Arduino.h>
#include <sntp.h>
#include <PolledTimeout.h>

static esp8266::polledTimeout::periodicFastMs poll(1000);

void setup() {
    Serial.begin(115200);
    String value("testtesttest");
    configTime("UTC0", value.c_str(), "domainname");
    Serial.printf("before: 0 -> %s, 1 -> %s\n", sntp_getservername(0), sntp_getservername(1));
    WiFi.persistent(false);
    WiFi.mode(WIFI_STA);
    WiFi.begin("blah", "blah");
}

void loop() {
    if (poll) {
        Serial.printf("1 -> %s\n", sntp_getservername(1));
        Serial.flush();
        Serial.printf("0 -> %s\n", sntp_getservername(0));
        Serial.flush();
    }
}

Debug Messages

// 2.6.3 even crashes. git manages to stay online

before: testtesttest domainname
1 -> domainname
0 -> ␔��?�:
1 -> domainname
0 -> ␔��?�:
1 -> domainname
0 -> ␔��?�:

Exception (28):
epc1=0x40207c2e epc2=0x00000000 epc3=0x00000000 excvaddr=0x00000000 depc=0x00000000
...

[devyte]

You're right, configTime saves a pointer to the received string. It was meant for const char strings, and not for temporary strings. For the rare cases where a configurable server is needed, the user can maintain the lifetime of the string.
Your case is a classic C code gotcha, it's mentioned in books even. I was inclined to just close this, but the internals can be enhanced without too much complexity to cover the use case, and thereby reduce the onus on the user, so marking as enhancement.
How about implementing it yourself and making a pr? Given the current repo workload, and low priority of this, I don't know when we'll get to it, but if you make a pr we can review it and merge sooner.

[mcspr]

I think my main problem was that it is never stated anywhere, const char * as a hint does not really say much since we have other functions for persistent services that accept it but copy internally.

Arduino/libraries/ArduinoOTA/ArduinoOTA.cpp

Lines 79 to 81 in 50a491b

	void ArduinoOTAClass::setHostname(const char * hostname) {
	if (!_initialized && !_hostname.length() && hostname) {
	_hostname = hostname;

Arduino/libraries/ArduinoOTA/ArduinoOTA.h

Line 74 in 50a491b

String _hostname;

Which can be the approach here, either having static String inside setServer(), or static String array[3] in time.cpp

Or, considering most common approach with hard-coded server string that is never changed, maybe just

• add comment to NTP-TZ sketch
• add comment to configTime
• add reference to configTime to documentation
  (which I assume cannot be generated from code comment?)

Which I sort-of like better.

edit: typos

[d-a-v]

This is still opened.
Is it going to be comments, or an array of Strings in the core ?

[mcspr]

Could be both, just to retain the ability to hold pointers manually.

// use this if your char pointers persist for the duration of the program. implementation detail!
void blah(const char* a, const char* b, const char* c);

// use this if your strings are deleted after this block ends
void blah(String a, String b, String c) {
  static String _a;
  static String _b;
  static String _c;
  _a = std::move(a);
  _b = std::move(b);
  _c = std::move(c);
  blah(_a.c_str(), _b.c_str(), _c.c_str());
}