WString: make to be safer init state, in order to tolerate (potential) stray dtor calls

[jjsuwa]

derived from #7553

[earlephilhower]

I don't see how this affects the c'tor/d'tor one way or the other. It just seems to change the name and inverting the logic for SSO flags.

If that flag itself is not set somewhere before being used in a d'tor, you have to assume it's random. So, still the 50% chance of trying to free something that wasn't allocated.

[jjsuwa]

I don't see how this affects the c'tor/d'tor one way or the other.

i think so - the idea do not belong to me :)

even w/o this PR, dtor do not call free() even if before global ctor runs:

void String::invalidate(void) {
    if(!isSSO() && wbuffer())
        free(wbuffer());
    init();
}

because wbuffer() returns nullptr in such case.
non-global (in stack) ctor interruption occurs? should panic() asap.

It just seems to change the name and inverting the logic for SSO flags.

a some other weak reasons:

• insn saving: only requires value of 0 (in contrast with both 0 and 128)
• it seems to be natural that the init state is wholly zeroed

If that flag itself is not set somewhere before being used in a d'tor, you have to assume it's random. So, still the 50% chance of trying to free something that wasn't allocated.

potential... (negligible in fact)

[TD-er]

potential... (negligible in fact)

Steps to reproduce it...

• Create a struct with several String objects in it.
• Make sure you're running out of memory ;)
• Create such a struct directly by adding it to a std::list using emplace_back, preferrably with some constructor values of the struct which will cause a memory allocation fail in one of the first members making sure the latter String members are not properly constructed.
• Try to erase the item from the list. => crash
• Edit your "negligible" statement here ;)

[earlephilhower]

@TD-er can you give an MCVE for that, one that fails for you?

Seems straightforward enough (and you can do a void *unused = malloc(47000); at the beginning to eat up most RAM and then keep adding strings until there is a failure. But, again, I think the issue is as likely to be in the std::list (which wants to throw an exception on OOM but can't in normal operating mode on the 8266) as in String.

[jjsuwa]

Make sure you're running out of memory ;)

post-OOM behavior... virtually impossible to keep heap integrity, should dump so to debug serial and panic() if possible.

[TD-er]

Make sure you're running out of memory ;)

post-OOM behavior... virtually impossible to keep heap integrity, should dump so to debug serial and panic() if possible.

Well I do always check large object allocations whether they return nullptr.
As my sketch is quite often running into almost out of memory situations, it is nice to have those be handled a bit more gracefully than a crash.
But this also means whatever member exists in a class must be able to be destructed regardless whether the class was fully constructed.

[TD-er]

@TD-er can you give an MCVE for that, one that fails for you?

Seems straightforward enough (and you can do a void *unused = malloc(47000); at the beginning to eat up most RAM and then keep adding strings until there is a failure. But, again, I think the issue is as likely to be in the std::list (which wants to throw an exception on OOM but can't in normal operating mode on the 8266) as in String.

I will try to find some time for it this weekend.

[jjsuwa]

As my sketch is quite often running into almost out of memory situations, it is nice to have those be handled a bit more gracefully than a crash.

post-OOM condition is typical corner-case and thus malloc() without nullptr check are so ubiquitous despite of bad manners.
but in your work, is not...

mentioned above, if BSS still keeps its consistency, global dtor do not call free() even if before global ctor runs.
if not, implies BSS corruption by no-OOM-checked codes everywhere (except for your code), and then the penalty have to be paid by all statically-cleanups not limited to String's one, such as Arduino core/libs, GCC runtime/newlibs, and binary blobs from espressif further...

this is just why virtually impossible.

the main interest: is this PR sufficient to resolve your problem?

[earlephilhower]

On further inspection, I'm really not seeing anything this changes and I'm not seeing how String() could free an invalid pointer and cause a crash like @TD-er is thinking.

My reasoning:

All String constructors call String::init():

Arduino/cores/esp8266/WString.cpp

Lines 131 to 135 in a460cb7

	inline void String::init(void) {
	setSSO(true);
	setLen(0);
	wbuffer()[0] = 0;
	}

If the constructor is called with a nullptr this, that will immediately crash as it is writing values to NULL.

So, the only way it doesn't crash is with a valid this pointer. It sets the isSSO flag and makes a 0-len string in the structure.

Now on destruction (which cannot ever happen if the c'tor this was nullptr) we do:

Arduino/cores/esp8266/WString.cpp

Lines 123 to 125 in a460cb7

	String::~String() {
	invalidate();
	}

Arduino/cores/esp8266/WString.cpp

Lines 137 to 141 in a460cb7

	void String::invalidate(void) {
	if(!isSSO() && wbuffer())
	free(wbuffer());
	init();
	}

So, we check the isSSO flag and potentially free a heap pointer and then do the same as the c'tor (i.e. set some flags which, in this case, are ignored).

So, this PR doesn't change anything or affect the behavior in any way. And, as far as I can see, the existing behavior is safe and sane.

If memory gets corrupted elsewhere, or the String this is corrupted by other code, well yeah it could crash but that's not String's fault or problem and there's nothing we can do about it.

[jjsuwa]

thanks for the discussion, this PR is melt back into #7553 and closed.