request to modify String::concat

[darrendavidhumphrey]

unsigned char String::concat(const char *cstr, unsigned int length) {
unsigned int newlen = len + length;
if(!cstr)
return 0;
if(length == 0)
return 1;
if(!reserve(newlen))
return 0;
strcpy(buffer + len, cstr);
len = newlen;
return 1;
}

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: [All ESPxx]
• Core Version: [latest]
• Development Env: [All]
• Operating System: [All]

Problem Description

The String (Wstring.cpp) class has no method for concat() from a non-null terminated string. This is annoying, and inffecient, when processing payloads from MQTT messages, which are not-null terminated.

This existing Concat(cstr,length) method would work with minor modifications, but it is protected. I am proposing modifying it as shown below and making it public.

unsigned char String::concat(const char *cstr, unsigned int length) {
unsigned int newlen = len + length;
if(!cstr)
return 0;
if(length == 0)
return 1;
if(!reserve(newlen))
return 0;

-- strcpy(buffer + len, cstr); // Would fail if cstr is not NULL terminated
++ memcpy(buffer + len, cstr,length);
++ buffer[newlen] = 0; // insert NULL terminator

len = newlen;
return 1;
}

MCVE Sketch

#include <Arduino.h>
#include "WString.h"

void setup() {
   char bigBuff[1000];

   int messageSize= ReadMQTT_Payload(bigBuff);
   
    String someString;

// How you have to do it now.  concat called for each character
   string.reserve(messageSize)
   for (int i=0; i < messageSize; i++)
      someString.concat(bigBuff[i];

// How you could do it with proposal.  Single call to concat
   someString.concat(bigBuff,messageSize);
}

void loop() {

}

Debug Messages

Debug messages go here

[earlephilhower]

Interesting idea. Why not submit a PR with the change? It's simple and means less work for the few maintainers, which means it's got a better chance of getting in there faster. :)

[darrendavidhumphrey]

I thought I was :) I haven't submitted anything to the project before.

…

On Sat, Aug 18, 2018 at 12:55 PM Earle F. Philhower, III < ***@***.***> wrote: Interesting idea. Why not submit a PR with the change? It's simple and means less work for the few maintainers, which means it's got a better chance of getting in there faster. :) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5061 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Ag3P26Xrkpj67GUl8GjK8woJfI7A-y-hks5uSEcPgaJpZM4WCoOo> .

[earlephilhower]

@darrendavidhumphrey You're almost there. This is an issue, but a PR is an actual code repo you want merged. Makes it easier for us, and makes it better for you since you get the honor (or blame, sometimes, believe me!) of having your name in the repo associated with the code change.

There's a good github description, but basically to make a PR you just
. From the web interface hit "fork this repo"
. git clone https://github.com/**your fork**
. git checkout -b new-pr-branch-name
<apply changes, do your testing>
. git commit -a
. git push
. Back at the web UI, a new notice will appear above your code asking if you're like to submit a PR. Hit the button, fill out a field or two, and we're set (CI will verify it compiles, some host tests will run, etc).

At that stage, the maintainers can just click one button and merge. We get one more contributor on the list, and have one less thing to worry about. :)

[darrendavidhumphrey]

Thanks for the tutorial :) I will submit a PR in the near future. Also I noticed your name on some the SSL code, so I was wondering if you can answer a general question for me. I'm trying to use WiFiClientSecure to connect to a website via HTTPS. I first started the code a while back when there was only WiFiClientSecure as an option. Now I notice there's BearSSL and AxTLS versions of those classes. I'm pretty sure I'm running out of memory with the standard WiFiClientSecure class, as it crashes when I call connect() after setting the certificates. It works fine with an unencrypted connection. Should I be using WiFiClientBearSSL or WiFiClientSecureAxTLS instead? I am a little confused at the difference between the various options. I will be connecting to amazon AWS if that makes a difference. I know Amazon has some minimum requirements for TLS version.

…

On Tue, Aug 21, 2018 at 6:06 PM Earle F. Philhower, III < ***@***.***> wrote: @darrendavidhumphrey <https://github.com/darrendavidhumphrey> You're almost there. This is an issue, but a PR is an actual code repo you want merged. Makes it easier for us, and makes it better for you since you get the honor (or blame, sometimes, believe me!) of having your name in the repo associated with the code change. There's a good github description, but basically to make a PR you just . From the web interface hit "fork this repo" . git clone https://github.com/**your fork** . git checkout -b new-pr-branch-name <apply changes, do your testing> . git commit -a . git push . Back at the web UI, a new notice will appear above your code asking if you're like to submit a PR. Hit the button, fill out a field or two, and we're set (CI will verify it compiles, some host tests will run, etc). At that stage, the maintainers can just click one button and merge. We get one more contributor on the list, and have one less thing to worry about. :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5061 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Ag3P27uovJw7iDnvl0QXHxQmta_v_6X6ks5uTISDgaJpZM4WCoOo> .

[earlephilhower]

BearSSL::WiFiClientSecure() is the one I'd recommend. It allocates memory all at once, so it is much less prone to OOM errors. It also is actively maintained so when bugs happen they get fixed, unlike axTLS.

[darrendavidhumphrey]

OK thanks, that's very helpful. That is the road I am going down right now. The good news is instead of just segfaulting, it's giving me an actual error message saying that it's out of memory. The bad news is I don't have enough free memory to use it. Is there any guidance on how much free heap is recommended? I'm running on ESP8266. I've got about 20K free before trying to start the SSL connection. Thanks, Darren

…

On Wed, Aug 22, 2018 at 11:57 AM Earle F. Philhower, III < ***@***.***> wrote: BearSSL::WiFiClientSecure() is the one I'd recommend. It allocates memory all at once, so it is much less prone to OOM errors. It also is actively maintained so when bugs happen they get fixed, unlike axTLS. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5061 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Ag3P29eOHwRajAN_2nBxmMhioNHl5-stks5uTX94gaJpZM4WCoOo> .

[earlephilhower]

It's been a little bit since I did measurements, but roughly the memory breakdown is:
. 4.5KB (only) on the first instance of BearSSL::client/server for the 2nd stack, present until all BearSSL objects are destroyed
. ~24KB (IIRC, could be a little higher) for each server/client connection. If you can use MFLN (see the examples, depends on the server supporting it) I think this can go down to ~8KB or so per connection. Again, though, only with MFLN to limit the TLS packet sizes.

[darrendavidhumphrey]

I actually did finally get it working. The problem was in the default send/receive buffer sizes. I looked through the code for WiFiClientSecure and found this line. setBufferSizes(16384, 512); // Minimum safe The receive buffer defaults to 16K, which is massive when running on ESP8266. Fortunately, I am connecting to my own REST API and I can control the maximum response size. In my code I call client->setBufferSizes(1500,512) which suffices for my needs. So far it seems to work OK. My only current issue is I am using SPIFFSCertStoreFile with "certs_from_mozilla.py" script to fetch the SSL certificates . It works, but it requires 175K of flash storage to store all those certificates, and it takes about 4 seconds to parse the certificate store at bootup. I am hoping I can prune the number of certificates for my application both to free up Flash space and to speed up booting the device. Thanks for the inputs and suggestions. Going the BearSSLSecureClient route seems to be paying off. Darren

…

On Thu, Aug 23, 2018 at 6:16 PM Earle F. Philhower, III < ***@***.***> wrote: It's been a little bit since I did measurements, but roughly the memory breakdown is: . 4.5KB (only) on the first instance of BearSSL::client/server for the 2nd stack, present until all BearSSL objects are destroyed . ~24KB (IIRC, could be a little higher) for each server/client connection. *If* you can use MFLN (see the examples, depends on the server supporting it) I think this can go down to ~8KB or so per connection. Again, though, only with MFLN to limit the TLS packet sizes. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5061 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Ag3P28Lk0zD1i-zA53uI9dyboXeFZU2bks5uTym3gaJpZM4WCoOo> .

[earlephilhower]

Great news! If you control both ends, the memory really can be reasonable. Just be aware, anything that doesn't fit will cause the SSL connection to completely drop. There is no fragmentation (w/o real MFLN) in TLS other than at that 16K chunksize.

I don't remember it taking 4 seconds to parse the certstore output before, but what I have found with SPIFFs is it can get into a very slow state sometimes.

The file is a simple UNIX .AR archive, so you can use any tool to make it with just your own certs, if desired. If you only have a single cert (say, self-signed CA) then just use that one and don't use a CertStore at all, just pass in the CA.