ESP8266 Updater class does not honour size parameter in begin(size)

[jason-but]

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
• Core Version: Latest auto install
• Development Env: Arduino IDE
• Operating System: Ubuntu

Settings in IDE

• Module: Generic ESP8266 Module
• Flash Mode: qio
• Flash Size: 4MB
• lwip Variant: v2 Lower Memory
• Reset Method: ck
• Flash Frequency: 40Mhz
• CPU Frequency: 80Mhz|
• Upload Using: SERIAL
• Upload Speed: 115200

Problem Description

There is an existing bug in the code for the Updater class that is still present in the latest GIT source available online. The error is in the write() method and more particularly the check for running out of space with:

if(len > remaining()){

I have been developing my own code to update OTA via a web server ESP8266WebServer that then calls Update.begin(), Update.write(), Update.end() and if I upload MORE bytes than I specify in begin(), write() does NOT trigger an error and continues writing to flash, eventually overwriting the SPIFFS directory.

This may be an issue with the _bufferSize = FLASH_SECTOR_SIZE = 4096 and the buffer sent to write being 2048 which are exact multiples of each other or it may be generically incorrect

The bug can be verified by walking through an example where _size = 5000, and manually considering what happens on each call to write() with a buffer of 2048

Call 1 -> 2048 bytes get stored in buffer, method returns 2048
Call 2 -> 2048 bytes get stored, buffer is full but NOT written yet, method returns 2048
Call 3 -> 2048 is now too many, but the test succeeds. Now the 4096 byte buffer gets flashed, the next 2048 bytes get stored in the buffer, and remaining() will forever more return a negative number (size_t being unsigned this means a huge number) and an unlimited amount of bytes can be written

I believe the test should be changed to check if written (progress() + bytes in buffer + len is greater than size

if (progress() + _bufferLen + len > _size) {

Why hasn't this been caught

It appears that most examples just use the:

    uint32_t maxSketchSpace = (ESP.getFreeSketchSpace() - 0x1000) & 0xFFFFF000;

to pass to begin() which means that the file will typically finish before the problem is seen

[earlephilhower]

Everything's an unsigned int in the object (size_t or uint32_t) so the subtraction past the end definitely results in a flip to MAXINT-epsilon and you can write as much as you want, potentially blowing up SPIFFS, I believe.

A simple solution which lets you write up to the end of the final flash sector (i.e. a spot that'd be overwritten anyway due to the erase operation) is to make them all int and the existing comparison will catch it (as well as any other missed codepaths).

The alternative would be to change the comparison (in all places made, something like 3-4 spots) to use something like @jason-but suggested.

[devyte]

Pushing back.