Correctly handle unaligned address in EspClass::flashWrite u8 overload (#8605)

Separate page handling logic and the actual writing. Make sure we place both unaligned src and dest into a buffer.
Fixes edge-case introduced for SPIFFS that exclusively works through unaligned flash write function.

This copies the behaviour of official RTOS port, but does not change the original spi_flash_write.

cores/esp8266/Esp.cpp

@@ -673,7 +673,46 @@ bool EspClass::flashEraseSector(uint32_t sector) {
     return rc == 0;
 }
 
+// Adapted from the old version of `flash_hal_write()` (before 3.0.0), which was used for SPIFFS to allow
+// writing from both unaligned u8 buffers and to an unaligned offset on flash.
+// Updated version re-uses some of the code from RTOS, replacing individual methods for block & page
+// writes with just a single one
+// https://github.com/espressif/ESP8266_RTOS_SDK/blob/master/components/spi_flash/src/spi_flash.c
+// (if necessary, we could follow the esp-idf code and introduce flash chip drivers controling more than just writing methods?)
+
+// This is a generic writer that does not cross page boundaries.
+// Offset, data address and size *must* be 4byte aligned.
+static SpiFlashOpResult spi_flash_write_page_break(uint32_t offset, uint32_t *data, size_t size) {
+    static constexpr uint32_t PageSize { FLASH_PAGE_SIZE };
+    size_t size_page_aligned = PageSize - (offset % PageSize);
+
+    // most common case, we don't cross a page and simply write the data
+    if (size < size_page_aligned) {
+        return spi_flash_write(offset, data, size);
+    }
+
+    // otherwise, write the initial part and continue writing breaking each page interval
+    SpiFlashOpResult result = SPI_FLASH_RESULT_ERR;
+    if ((result = spi_flash_write(offset, data, size_page_aligned)) != SPI_FLASH_RESULT_OK) {
+        return result;
+    }
+
+    const auto last_page = (size - size_page_aligned) / PageSize;
+    for (uint32_t page = 0; page < last_page; ++page) {
+        if ((result = spi_flash_write(offset + size_page_aligned, data + (size_page_aligned >> 2), PageSize)) != SPI_FLASH_RESULT_OK) {
+            return result;
+        }
+
+        size_page_aligned += PageSize;
+    }
+
+    // finally, the remaining data
+    return spi_flash_write(offset + size_page_aligned, data + (size_page_aligned >> 2), size - size_page_aligned);
+}
+
 #if PUYA_SUPPORT
+// Special wrapper for spi_flash_write *only for PUYA flash chips*
+// Already handles paging, could be used as a `spi_flash_write_page_break` replacement
 static SpiFlashOpResult spi_flash_write_puya(uint32_t offset, uint32_t *data, size_t size) {
     if (data == nullptr) {
       return SPI_FLASH_RESULT_ERR;
@@ -720,195 +759,129 @@ static SpiFlashOpResult spi_flash_write_puya(uint32_t offset, uint32_t *data, si
 }
 #endif
 
-bool EspClass::flashReplaceBlock(uint32_t address, const uint8_t *value, uint32_t byteCount) {
+static constexpr size_t Alignment { 4 };
-    uint32_t alignedAddress = (address & ~3);
-    uint32_t alignmentOffset = address - alignedAddress;
 
-    if (alignedAddress != ((address + byteCount - 1) & ~3)) {
+template <typename T>
-        // Only one 4 byte block is supported
+static T aligned(T value) {
-        return false;
+    static constexpr auto Mask = Alignment - 1;
+    return (value + Mask) & ~Mask;
 }
-#if PUYA_SUPPORT
+
-    if (getFlashChipVendorId() == SPI_FLASH_VENDOR_PUYA) {
+template <typename T>
-        uint8_t tempData[4] __attribute__((aligned(4)));
+static T alignBefore(T value) {
-        if (spi_flash_read(alignedAddress, (uint32_t *)tempData, 4) != SPI_FLASH_RESULT_OK) {
+    return aligned(value) - Alignment;
-            return false;
 }
-        for (size_t i = 0; i < byteCount; i++) {
+
-            tempData[i + alignmentOffset] &= value[i];
+static bool isAlignedAddress(uint32_t address) {
+    return (address & (Alignment - 1)) == 0;
 }
-        if (spi_flash_write(alignedAddress, (uint32_t *)tempData, 4) != SPI_FLASH_RESULT_OK) {
+
-            return false;
+static bool isAlignedSize(size_t size) {
+    return (size & (Alignment - 1)) == 0;
 }
+
+static bool isAlignedPointer(const uint8_t *ptr) {
+    return isAlignedAddress(reinterpret_cast<uint32_t>(ptr));
 }
-    else
+
-#endif // PUYA_SUPPORT
-    {
-        uint32_t tempData;
-        if (spi_flash_read(alignedAddress, &tempData, 4) != SPI_FLASH_RESULT_OK) {
-            return false;
-        }
-        memcpy((uint8_t *)&tempData + alignmentOffset, value, byteCount);
-        if (spi_flash_write(alignedAddress, &tempData, 4) != SPI_FLASH_RESULT_OK) {
-            return false;
-        }
-    }
-    return true;
-}
 
 size_t EspClass::flashWriteUnalignedMemory(uint32_t address, const uint8_t *data, size_t size) {
-    size_t sizeLeft = (size & ~3);
+    auto flash_write = [](uint32_t address, uint8_t *data, size_t size) {
-    size_t currentOffset = 0;
+        return spi_flash_write(address, reinterpret_cast<uint32_t *>(data), size) == SPI_FLASH_RESULT_OK;
-    // Memory is unaligned, so we need to copy it to an aligned buffer
+    };
-    uint32_t alignedData[FLASH_PAGE_SIZE / sizeof(uint32_t)] __attribute__((aligned(4)));
-    // Handle page boundary
-    bool pageBreak = ((address % 4) != 0) && ((address / FLASH_PAGE_SIZE) != ((address + sizeLeft - 1) / FLASH_PAGE_SIZE));
 
-    if (pageBreak) {
+    auto flash_read = [](uint32_t address, uint8_t *data, size_t size) {
-        size_t byteCount = 4 - (address % 4);
+        return spi_flash_read(address, reinterpret_cast<uint32_t *>(data), size) == SPI_FLASH_RESULT_OK;
+    };
 
-        if (!flashReplaceBlock(address, data, byteCount)) {
+    constexpr size_t BufferSize { FLASH_PAGE_SIZE };
+    alignas(alignof(uint32_t)) uint8_t buf[BufferSize];
+
+    size_t written = 0;
+
+    if (!isAlignedAddress(address)) {
+        auto before_address = alignBefore(address);
+        auto offset = address - before_address;
+        auto wlen = std::min(Alignment - offset, size);
+
+        if (!flash_read(before_address, &buf[0], Alignment)) {
             return 0;
         }
-        // We will now have aligned address, so we can cross page boundaries
-        currentOffset += byteCount;
-        // Realign size to 4
-        sizeLeft = (size - byteCount) & ~3;
-    }
 
-    while (sizeLeft) {
+#if PUYA_SUPPORT
-        size_t willCopy = std::min(sizeLeft, sizeof(alignedData));
+        if (getFlashChipVendorId() == SPI_FLASH_VENDOR_PUYA) {
-        memcpy(alignedData, data + currentOffset, willCopy);
+            for (size_t i = 0; i < wlen ; ++i) {
-        // We now have address, data and size aligned to 4 bytes, so we can use aligned write
+                buf[offset + i] &= data[i];
-        if (!flashWrite(address + currentOffset, alignedData, willCopy))
+            }
-        {
+        } else {
+#endif
+            memcpy(&buf[offset], data, wlen);
+#if PUYA_SUPPORT
+        }
+#endif
+
+        if (!flash_write(before_address, &buf[0], Alignment)) {
             return 0;
         }
-        sizeLeft -= willCopy;
+
-        currentOffset += willCopy;
+        address += wlen;
+        data += wlen;
+        written += wlen;
+        size -= wlen;
     }
 
-    return currentOffset;
+    while (size > 0) {
+        auto len = std::min(size, BufferSize);
+        auto wlen = aligned(len);
+
+        if (wlen != len) {
+            auto partial = wlen - Alignment;
+            if (!flash_read(address + partial, &buf[partial], Alignment)) {
+                return written;
+            }
         }
 
-bool EspClass::flashWritePageBreak(uint32_t address, const uint8_t *data, size_t size) {
+        memcpy(&buf[0], data, len);
-    if (size > 4) {
+        if (!flashWrite(address, reinterpret_cast<const uint32_t *>(&buf[0]), wlen)) {
-        return false;
+            return written;
-    }
-    size_t pageLeft = FLASH_PAGE_SIZE - (address % FLASH_PAGE_SIZE);
-    size_t offset = 0;
-    size_t sizeLeft = size;
-    if (pageLeft > 3) {
-        return false;
         }
 
-    if (!flashReplaceBlock(address, data, pageLeft)) {
+        address += len;
-        return false;
+        data += len;
+        written += len;
+        size -= len;
     }
-    offset += pageLeft;
+
-    sizeLeft -= pageLeft;
+    return written;
-    // We replaced last 4-byte block of the page, now we write the remainder in next page
-    if (!flashReplaceBlock(address + offset, data + offset, sizeLeft)) {
-        return false;
-    }
-    return true;
 }
 
 bool EspClass::flashWrite(uint32_t address, const uint32_t *data, size_t size) {
-    SpiFlashOpResult rc = SPI_FLASH_RESULT_OK;
+    SpiFlashOpResult result;
-    bool pageBreak = ((address % 4) != 0 && (address / FLASH_PAGE_SIZE) != ((address + size - 1) / FLASH_PAGE_SIZE));
-
-    if ((uintptr_t)data % 4 != 0 || size % 4 != 0 || pageBreak) {
-        return false;
-    }
 #if PUYA_SUPPORT
     if (getFlashChipVendorId() == SPI_FLASH_VENDOR_PUYA) {
-        rc = spi_flash_write_puya(address, const_cast<uint32_t *>(data), size);
+        result = spi_flash_write_puya(address, const_cast<uint32_t *>(data), size);
     }
     else
-#endif // PUYA_SUPPORT
+#endif
     {
-        rc = spi_flash_write(address, const_cast<uint32_t *>(data), size);
+        result = spi_flash_write_page_break(address, const_cast<uint32_t *>(data), size);
     }
-    return rc == SPI_FLASH_RESULT_OK;
+    return result == SPI_FLASH_RESULT_OK;
 }
 
 bool EspClass::flashWrite(uint32_t address, const uint8_t *data, size_t size) {
-    if (size == 0) {
+    if (data && size) {
-        return true;
+        if (!isAlignedAddress(address)
+         || !isAlignedPointer(data)
+         || !isAlignedSize(size))
+        {
+            return flashWriteUnalignedMemory(address, data, size) == size;
         }
 
-    size_t sizeLeft = size & ~3;
+        return flashWrite(address, reinterpret_cast<const uint32_t *>(data), size);
-    size_t currentOffset = 0;
-
-    if (sizeLeft) {
-        if ((uintptr_t)data % 4 != 0) {
-            size_t written = flashWriteUnalignedMemory(address, data, size);
-            if (!written) {
-                return false;
-            }
-            currentOffset += written;
-            sizeLeft -= written;
-        } else {
-            bool pageBreak = ((address % 4) != 0 && (address / FLASH_PAGE_SIZE) != ((address + sizeLeft - 1) / FLASH_PAGE_SIZE));
-
-            if (pageBreak) {
-                while (sizeLeft) {
-                    // We cannot cross page boundary, but the write must be 4 byte aligned,
-                    // so this is the maximum amount we can write
-                    size_t pageBoundary = (FLASH_PAGE_SIZE - ((address + currentOffset) % FLASH_PAGE_SIZE)) & ~3;
-
-                    if (sizeLeft > pageBoundary) {
-                        // Aligned write up to page boundary
-                        if (!flashWrite(address + currentOffset, (uint32_t *)(data + currentOffset), pageBoundary)) {
-                            return false;
-                        }
-                        currentOffset += pageBoundary;
-                        sizeLeft -= pageBoundary;
-                        // Cross the page boundary
-                        if (!flashWritePageBreak(address + currentOffset, data + currentOffset, 4)) {
-                            return false;
-                        }
-                        currentOffset += 4;
-                        sizeLeft -= 4;
-                    } else {
-                        // We do not cross page boundary
-                        if (!flashWrite(address + currentOffset, (uint32_t *)(data + currentOffset), sizeLeft)) {
-                            return false;
-                        }
-                        currentOffset += sizeLeft;
-                        sizeLeft = 0;
-                    }
-                }
-            } else {
-                // Pointer is properly aligned and write does not cross page boundary,
-                // so use aligned write
-                if (!flashWrite(address, (uint32_t *)data, sizeLeft)) {
-                    return false;
-                }
-                currentOffset = sizeLeft;
-                sizeLeft = 0;
-            }
-        }
-    }
-    sizeLeft = size - currentOffset;
-    if (sizeLeft > 0) {
-        // Size was not aligned, so we have some bytes left to write, we also need to recheck for
-        // page boundary crossing
-        bool pageBreak = ((address % 4) != 0 && (address / FLASH_PAGE_SIZE) != ((address + sizeLeft - 1) / FLASH_PAGE_SIZE));
-
-        if (pageBreak) {
-            // Cross the page boundary
-            if (!flashWritePageBreak(address + currentOffset, data + currentOffset, sizeLeft)) {
-                return false;
-            }
-        } else {
-            // Just write partial block
-            flashReplaceBlock(address + currentOffset, data + currentOffset, sizeLeft);
-        }
     }
 
-    return true;
+    return false;
 }
 
 bool EspClass::flashRead(uint32_t address, uint8_t *data, size_t size) {
@@ -916,23 +889,24 @@ bool EspClass::flashRead(uint32_t address, uint8_t *data, size_t size) {
     size_t currentOffset = 0;
 
     if ((uintptr_t)data % 4 != 0) {
-        uint32_t alignedData[FLASH_PAGE_SIZE / sizeof(uint32_t)] __attribute__((aligned(4)));
+        constexpr size_t BufferSize { FLASH_PAGE_SIZE / sizeof(uint32_t) };
+        alignas(alignof(uint32_t)) uint32_t buf[BufferSize];
         size_t sizeLeft = sizeAligned;
 
         while (sizeLeft) {
-            size_t willCopy = std::min(sizeLeft, sizeof(alignedData));
+            size_t willCopy = std::min(sizeLeft, BufferSize);
             // We read to our aligned buffer and then copy to data
-            if (!flashRead(address + currentOffset, alignedData, willCopy))
+            if (!flashRead(address + currentOffset, &buf[0], willCopy))
             {
                 return false;
             }
-            memcpy(data + currentOffset, alignedData, willCopy);
+            memcpy(data + currentOffset, &buf[0], willCopy);
             sizeLeft -= willCopy;
             currentOffset += willCopy;
         }
     } else {
         // Pointer is properly aligned, so use aligned read
-        if (!flashRead(address, (uint32_t *)data, sizeAligned)) {
+        if (!flashRead(address, reinterpret_cast<uint32_t *>(data), sizeAligned)) {
             return false;
         }
         currentOffset = sizeAligned;
@@ -940,10 +914,10 @@ bool EspClass::flashRead(uint32_t address, uint8_t *data, size_t size) {
 
     if (currentOffset < size) {
         uint32_t tempData;
-        if (spi_flash_read(address + currentOffset, &tempData, 4) != SPI_FLASH_RESULT_OK) {
+        if (spi_flash_read(address + currentOffset, &tempData, sizeof(tempData)) != SPI_FLASH_RESULT_OK) {
             return false;
         }
-        memcpy((uint8_t *)data + currentOffset, &tempData, size - currentOffset);
+        memcpy(data + currentOffset, &tempData, size - currentOffset);
     }
 
     return true;

cores/esp8266/Esp.h

@@ -252,39 +252,18 @@ class EspClass {
          */
         static void resetHeap();
     private:
-        /**
-         * @brief Replaces @a byteCount bytes of a 4 byte block on flash
-         *
-         * @param address flash address
-         * @param value buffer with data
-         * @param byteCount number of bytes to replace
-         * @return bool result of operation
-         * @retval true success
-         * @retval false failed to read/write or invalid args
-         */
-        static bool flashReplaceBlock(uint32_t address, const uint8_t *value, uint32_t byteCount);
         /**
          * @brief Write up to @a size bytes from @a data to flash at @a address
-         * This function takes case of unaligned memory access by copying @a data to a temporary buffer,
+         * This function handles all cases of unaligned memory acccess; when either
-         * it also takes care of page boundary crossing see @a flashWritePageBreak as to why it's done.
+         * address is not aligned, data pointer is not aligned or size is not a multiple of 4.
-         * Less than @a size bytes may be written, due to 4 byte alignment requirement of spi_flash_write
+         * User of this function should note that @a data will be copied into a buffer allocated on stack.
+         *
          * @param address address on flash where write should start
          * @param data input buffer
          * @param size amount of data
          * @return size_t amount of data written, 0 on failure
          */
         static size_t flashWriteUnalignedMemory(uint32_t address, const uint8_t *data, size_t size);
-        /**
-         * @brief Splits up to 4 bytes into 4 byte blocks and writes them to flash
-         * We need this since spi_flash_write cannot handle writing over a page boundary with unaligned offset
-         * i.e. spi_flash_write(254, data, 4) will fail, also we cannot write less bytes as in
-         * spi_flash_write(254, data, 2) since it will be extended internally to 4 bytes and fail
-         * @param address start of write
-         * @param data data to be written
-         * @param size amount of data, must be < 4
-         * @return bool result of operation
-         */
-        static bool flashWritePageBreak(uint32_t address, const uint8_t *data, size_t size);
 };
 
 extern EspClass ESP;

tests/device/test_spi_flash/test_spi_flash.ino

@@ -1,5 +1,4 @@
 #include <BSTest.h>
-#include <ESP8266WiFi.h>
 
 BS_ENV_DECLARE();
 
@@ -176,6 +175,11 @@ TEST_CASE("Unaligned page cross only", "[spi_flash]")
     CHECK(testFlash(0xa0000 + 255, 1, 2));
 }
 
+TEST_CASE("Unaligned page cross with unaligned size (#8372, #8588, #8605)", "[spi_flash]")
+{
+    CHECK(testFlash(0xa00b, 0, 202));
+}
+
 void loop ()
 {
 }