base64::encode crashes for small buffers

[LaborEtArs]

String base64::encode(uint8_t * data, size_t length, bool doNewLines) will crash when used with a quite short input buffer; for example 4 bytes.
A 4 byte input to base64 will produce an 8 byte output (plus trailing '\0' = 9), as two filling bytes will by attached to the input before processing. The buffer calculated by base64::encode however is only 4 * 1.6f + 1 = 6.4 + 1 = 6 + 1 = 7 bytes long!
A better size algorithm would be:
size_t size = ((((length * 4) + 2) / 3) + (3 - (length % 3)) + 1); if (doNewLines) size += ((size + 71) / 72);

(((length * 4) + 2) / 3):
3 input bytes (24 bits) will be converted to 4 output bytes (4 * 6bits = 24bit); the +2 ensures the division to be the next ceiling integer

(3 - (length % 3)):
Input buffers that can't be divided by 3 will be filled up by the algorithm

1:
Trailing '\0'

((size + 71) / 72):
If 'doNewLines' is true, every 72 (encoded) chars a newline will be added (again the +71 ensures the division to be the next ceiling integer)

[earlephilhower]

A little more readable, but doing the same thing for the case in question a couple other lengths I tried:

size_t size = ((len + 2) / 3 * 4) + 1;
if (doNewLines) {
  size += ((size + 71) / 72);
}

Would you like to submit a pull request to fix this?

[LaborEtArs]

Your code is better than mine!
As , in fact, I'm a github noob (therefor the problems formatting the code), I've got no idea how to do a pull request :-(
Maybe somebody else (you?) could do it :-)

[d-a-v]

@LaborEtArs Are you using latest master ?
This formula has been fixed by another user few weeks ago, and with 4 I get 8.
I'm not saying it's rock solid, but at least it does not agree with your statements (which is 4 gives 7).
see #4786

[LaborEtArs]

Before the change, the code was:

/**
 * convert input data to base64
 * @param data uint8_t *
 * @param length size_t
 * @return String
 */
String base64::encode(uint8_t * data, size_t length, bool doNewLines) {
    // base64 needs more size then the source data
    size_t size = ((length * 1.6f) + 1);
    char * buffer = (char *) malloc(size);
    if(buffer) {

which is the same as here on github

[LaborEtArs]

Sorry, I'm using a IE10 right now; having problems with the github site... so can't insert links...
However, the above code form (https://raw.githubusercontent.com/esp8266/Arduino/master/cores/esp8266/base64.cpp) gives 7 for an input buffer of 8 bytes.
But earlephilhower's code is more straightforward than mine anyway :-)

[d-a-v]

Sorry, I had no knowledge of this duplicate length calculation code.
String is using libb64, and libb64 has its own length calculator.
Best IMO is to check the already existing one, fix it if necessary, and use it.

[LaborEtArs]

The updated macros in cencode.h are OK; same results as earlephilhower's code for length = 0..1024 :-)
As cencode.h is included in base64.cpp anyway, I would propose to use these macros there. I will try to create a pull request.

[LaborEtArs]

Sorry, failed to create a PR; do I really have to create a branch of the repo just for such a small change???

[d-a-v]

Yes :)

[devyte]

Fork the repo only once.
Clone your fork locally only once.
New branch in your local git for each PR.
Commit locally, then push to your fork.
A green button will appear in the repo webpage, from which you can create the PR.

[LaborEtArs]

Forking worked, Local cloning failed two times with:
fatal: unable to access 'https://git.savannah.nongnu.org/git/lwip.git/': Failed to connect to git.savannah.nongnu.org port 443: Operation timed out
fatal: clone of 'https://git.savannah.nongnu.org/git/lwip.git' into submodule path '/Users/hartmut/Documents/GitHub/Arduino/tools/sdk/lwip2/builder/lwip2-src' failed
Failed to clone 'lwip2-src' a second time, aborting
Failed to recurse into submodule path 'tools/sdk/lwip2/builder'

Will try again next week...

[LaborEtArs]

Pull Request seems to be accepted (and merged) by now, but I can't find the change when looking into 'base64.cpp'...
In fact, github seems to be too sophisticated for me ;-)

[devyte]

#4934 is now merged.