Skip to content

Commit 78a1a66

Browse files
earlephilhowerearlephilhower
authored
Make SSO support \0s, use memmove, add test (esp8266#6155)
Supercedes esp8266#6027 Make SSO more generic by keeping track of its length explicitly, allowing for embedded \0s to exist in the String (just like the non-SSO ones). Use memmove/memcpy_P when we know the length of a string to save CPU time. Add tests to inject \0s in a String to ensure it is still working as designed.
1 parent 7910121 commit 78a1a66

File tree

3 files changed

+90
-42
lines changed

3 files changed

+90
-42
lines changed

‎cores/esp8266/WString.cpp

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@
3131

3232
String::String(const char *cstr) {
3333
init();
34-
if(cstr)
34+
if (cstr)
3535
copy(cstr, strlen(cstr));
3636
}
3737

@@ -136,7 +136,7 @@ inline void String::init(void) {
136136
}
137137

138138
void String::invalidate(void) {
139-
if(!sso() && wbuffer())
139+
if(!isSSO() && wbuffer())
140140
free(wbuffer());
141141
init();
142142
}
@@ -154,17 +154,21 @@ unsigned char String::reserve(unsigned int size) {
154154

155155
unsigned char String::changeBuffer(unsigned int maxStrLen) {
156156
// Can we use SSO here to avoid allocation?
157-
if (maxStrLen < sizeof(sso_buf)) {
158-
if (sso() || !buffer()) {
157+
if (maxStrLen < sizeof(sso.buff) - 1) {
158+
if (isSSO() || !buffer()) {
159159
// Already using SSO, nothing to do
160+
uint16_t oldLen = len();
160161
setSSO(true);
162+
setLen(oldLen);
161163
return 1;
162-
} else { // if bufptr && !sso()
163-
// Using bufptr, need to shrink into sso_buff
164-
char temp[sizeof(sso_buf)];
164+
} else { // if bufptr && !isSSO()
165+
// Using bufptr, need to shrink into sso.buff
166+
char temp[sizeof(sso.buff)];
165167
memcpy(temp, buffer(), maxStrLen);
166168
free(wbuffer());
169+
uint16_t oldLen = len();
167170
setSSO(true);
171+
setLen(oldLen);
168172
memcpy(wbuffer(), temp, maxStrLen);
169173
return 1;
170174
}
@@ -176,12 +180,12 @@ unsigned char String::changeBuffer(unsigned int maxStrLen) {
176180
return false;
177181
}
178182
uint16_t oldLen = len();
179-
char *newbuffer = (char *) realloc(sso() ? nullptr : wbuffer(), newSize);
180-
if(newbuffer) {
183+
char *newbuffer = (char *) realloc(isSSO() ? nullptr : wbuffer(), newSize);
184+
if (newbuffer) {
181185
size_t oldSize = capacity() + 1; // include NULL.
182-
if (sso()) {
186+
if (isSSO()) {
183187
// Copy the SSO buffer into allocated space
184-
memcpy(newbuffer, sso_buf, sizeof(sso_buf));
188+
memmove(newbuffer, sso.buff, sizeof(sso.buff));
185189
}
186190
if (newSize > oldSize)
187191
{
@@ -206,7 +210,7 @@ String & String::copy(const char *cstr, unsigned int length) {
206210
return *this;
207211
}
208212
setLen(length);
209-
strcpy(wbuffer(), cstr);
213+
memmove(wbuffer(), cstr, length + 1);
210214
return *this;
211215
}
212216

@@ -216,28 +220,28 @@ String & String::copy(const __FlashStringHelper *pstr, unsigned int length) {
216220
return *this;
217221
}
218222
setLen(length);
219-
strcpy_P(wbuffer(), (PGM_P)pstr);
223+
memcpy_P(wbuffer(), (PGM_P)pstr, length + 1); // We know wbuffer() cannot ever be in PROGMEM, so memcpy safe here
220224
return *this;
221225
}
222226

223227
#ifdef __GXX_EXPERIMENTAL_CXX0X__
224228
void String::move(String &rhs) {
225229
if(buffer()) {
226230
if(capacity() >= rhs.len()) {
227-
strcpy(wbuffer(), rhs.buffer());
231+
memmove(wbuffer(), rhs.buffer(), rhs.length() + 1);
228232
setLen(rhs.len());
229233
rhs.invalidate();
230234
return;
231235
} else {
232-
if (!sso()) {
236+
if (!isSSO()) {
233237
free(wbuffer());
234238
setBuffer(nullptr);
235239
}
236240
}
237241
}
238-
if (rhs.sso()) {
242+
if (rhs.isSSO()) {
239243
setSSO(true);
240-
memmove(sso_buf, rhs.sso_buf, sizeof(sso_buf));
244+
memmove(sso.buff, rhs.sso.buff, sizeof(sso.buff));
241245
} else {
242246
setSSO(false);
243247
setBuffer(rhs.wbuffer());
@@ -309,7 +313,7 @@ unsigned char String::concat(const String &s) {
309313
return 1;
310314
if (!reserve(newlen))
311315
return 0;
312-
memcpy(wbuffer() + len(), buffer(), len());
316+
memmove(wbuffer() + len(), buffer(), len());
313317
setLen(newlen);
314318
wbuffer()[len()] = 0;
315319
return 1;
@@ -326,7 +330,7 @@ unsigned char String::concat(const char *cstr, unsigned int length) {
326330
return 1;
327331
if(!reserve(newlen))
328332
return 0;
329-
strcpy(wbuffer() + len(), cstr);
333+
memmove(wbuffer() + len(), cstr, length + 1);
330334
setLen(newlen);
331335
return 1;
332336
}
@@ -392,7 +396,7 @@ unsigned char String::concat(const __FlashStringHelper * str) {
392396
if (length == 0) return 1;
393397
unsigned int newlen = len() + length;
394398
if (!reserve(newlen)) return 0;
395-
strcpy_P(wbuffer() + len(), (PGM_P)str);
399+
memcpy_P(wbuffer() + len(), (PGM_P)str, length + 1);
396400
setLen(newlen);
397401
return 1;
398402
}

‎cores/esp8266/WString.h

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -250,27 +250,29 @@ class String {
250250
uint16_t cap;
251251
uint16_t len;
252252
};
253-
254-
// SSO is handled by checking the last byte of sso_buff.
255-
// When not in SSO mode, that byte is set to 0xff, while when in SSO mode it is always 0x00 (so it can serve as the string terminator as well as a flag)
256-
// This allows strings up up to 12 (11 + \0 termination) without any extra space.
257-
enum { SSOSIZE = sizeof(struct _ptr) + 4 }; // Characters to allocate space for SSO, must be 12 or more
258-
enum { CAPACITY_MAX = 65535 }; // If size of capacity changed, be sure to update this enum
253+
// This allows strings up up to 11 (10 + \0 termination) without any extra space.
254+
enum { SSOSIZE = sizeof(struct _ptr) + 4 - 1 }; // Characters to allocate space for SSO, must be 12 or more
255+
struct _sso {
256+
char buff[SSOSIZE];
257+
unsigned char len : 7; // Ensure only one byte is allocated by GCC for the bitfields
258+
unsigned char isSSO : 1;
259+
} __attribute__((packed)); // Ensure that GCC doesn't expand the flag byte to a 32-bit word for alignment issues
260+
enum { CAPACITY_MAX = 65535 }; // If typeof(cap) changed from uint16_t, be sure to update this enum to the max value storable in the type
259261
union {
260262
struct _ptr ptr;
261-
char sso_buf[SSOSIZE];
263+
struct _sso sso;
262264
};
263265
// Accessor functions
264-
inline bool sso() const { return sso_buf[SSOSIZE - 1] == 0; }
265-
inline unsigned int len() const { return sso() ? strlen(sso_buf) : ptr.len; }
266-
inline unsigned int capacity() const { return sso() ? SSOSIZE - 1 : ptr.cap; }
267-
inline void setSSO(bool sso) { sso_buf[SSOSIZE - 1] = sso ? 0x00 : 0xff; }
268-
inline void setLen(int len) { if (!sso()) ptr.len = len; }
269-
inline void setCapacity(int cap) { if (!sso()) ptr.cap = cap; }
270-
inline void setBuffer(char *buff) { if (!sso()) ptr.buff = buff; }
266+
inline bool isSSO() const { return sso.isSSO; }
267+
inline unsigned int len() const { return isSSO() ? sso.len : ptr.len; }
268+
inline unsigned int capacity() const { return isSSO() ? (unsigned int)SSOSIZE - 1 : ptr.cap; } // Size of max string not including terminal NUL
269+
inline void setSSO(bool set) { sso.isSSO = set; }
270+
inline void setLen(int len) { if (isSSO()) sso.len = len; else ptr.len = len; }
271+
inline void setCapacity(int cap) { if (!isSSO()) ptr.cap = cap; }
272+
inline void setBuffer(char *buff) { if (!isSSO()) ptr.buff = buff; }
271273
// Buffer accessor functions
272-
inline const char *buffer() const { return (const char *)(sso() ? sso_buf : ptr.buff); }
273-
inline char *wbuffer() const { return sso() ? const_cast<char *>(sso_buf) : ptr.buff; } // Writable version of buffer
274+
inline const char *buffer() const { return (const char *)(isSSO() ? sso.buff : ptr.buff); }
275+
inline char *wbuffer() const { return isSSO() ? const_cast<char *>(sso.buff) : ptr.buff; } // Writable version of buffer
274276

275277
protected:
276278
void init(void);

‎tests/host/core/test_string.cpp

Lines changed: 48 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -320,11 +320,11 @@ TEST_CASE("String SSO works", "[core][String]")
320320
REQUIRE(s.c_str() == savesso);
321321
REQUIRE(s == "0123456789");
322322
REQUIRE(s.length() == 10);
323-
s += "a";
324-
REQUIRE(s.c_str() == savesso);
325-
REQUIRE(s == "0123456789a");
326-
REQUIRE(s.length() == 11);
327323
if (sizeof(savesso) == 4) {
324+
s += "a";
325+
REQUIRE(s.c_str() != savesso);
326+
REQUIRE(s == "0123456789a");
327+
REQUIRE(s.length() == 11);
328328
s += "b";
329329
REQUIRE(s.c_str() != savesso);
330330
REQUIRE(s == "0123456789ab");
@@ -334,12 +334,16 @@ TEST_CASE("String SSO works", "[core][String]")
334334
REQUIRE(s == "0123456789abc");
335335
REQUIRE(s.length() == 13);
336336
} else {
337+
s += "a";
338+
REQUIRE(s.c_str() == savesso);
339+
REQUIRE(s == "0123456789a");
340+
REQUIRE(s.length() == 11);
337341
s += "bcde";
338342
REQUIRE(s.c_str() == savesso);
339343
REQUIRE(s == "0123456789abcde");
340344
REQUIRE(s.length() == 15);
341345
s += "fghi";
342-
REQUIRE(s.c_str() == savesso);
346+
REQUIRE(s.c_str() != savesso);
343347
REQUIRE(s == "0123456789abcdefghi");
344348
REQUIRE(s.length() == 19);
345349
s += "j";
@@ -360,7 +364,7 @@ TEST_CASE("String SSO works", "[core][String]")
360364
REQUIRE(s.length() == 23);
361365
s += "nopq";
362366
REQUIRE(s.c_str() != savesso);
363-
REQUIRE(s == "0123456789abcdefghijklmnopq");
367+
REQUIRE(s == "0123456789abcdefghijklmnopq");
364368
REQUIRE(s.length() == 27);
365369
s += "rstu";
366370
REQUIRE(s.c_str() != savesso);
@@ -452,3 +456,41 @@ TEST_CASE("Issue #2736 - StreamString SSO fix", "[core][StreamString]")
452456
s.print('\"');
453457
REQUIRE(s == "{\"message\"");
454458
}
459+
460+
TEST_CASE("Strings with NULs", "[core][String]")
461+
{
462+
// The following should never be done in a real app! This is only to inject 0s in the middle of a string.
463+
// Fits in SSO...
464+
String str("01234567");
465+
REQUIRE(str.length() == 8);
466+
char *ptr = (char *)str.c_str();
467+
ptr[3] = 0;
468+
String str2;
469+
str2 = str;
470+
REQUIRE(str2.length() == 8);
471+
// Needs a buffer pointer
472+
str = "0123456789012345678901234567890123456789";
473+
ptr = (char *)str.c_str();
474+
ptr[3] = 0;
475+
str2 = str;
476+
REQUIRE(str2.length() == 40);
477+
String str3("a");
478+
ptr = (char *)str3.c_str();
479+
*ptr = 0;
480+
REQUIRE(str3.length() == 1);
481+
str3 += str3;
482+
REQUIRE(str3.length() == 2);
483+
str3 += str3;
484+
REQUIRE(str3.length() == 4);
485+
str3 += str3;
486+
REQUIRE(str3.length() == 8);
487+
str3 += str3;
488+
REQUIRE(str3.length() == 16);
489+
str3 += str3;
490+
REQUIRE(str3.length() == 32);
491+
str3 += str3;
492+
REQUIRE(str3.length() == 64);
493+
static char zeros[64] = {0};
494+
const char *p = str3.c_str();
495+
REQUIRE(!memcmp(p, zeros, 64));
496+
}

0 commit comments

Comments
 (0)