Five independent simulator correctness fixes (memory safety, stubbed NimBLE helpers, flash-sleep guard)

CMakeLists.txt

@@ -29,6 +29,12 @@ else()
 endif()
 set(CMAKE_C_STANDARD 11)#C11
 
+# The firmware targets ARM, where `char` is unsigned; on x86 it is signed, so
+# code that stores byte values in a plain `char` (or compares one to a value
+# above 127) behaves differently in the simulator than on the watch. Match the
+# hardware so the sim reproduces the firmware's arithmetic.
+add_compile_options(-funsigned-char)
+
 # copy lv_conf.h from InfiniTime project and do little modifications
 file(READ "${InfiniTime_DIR}/src/libs/lv_conf.h" lv_conf_main)
 # set '#define LV_TICK_CUSTOM     0'

sim/displayapp/LittleVgl.cpp

@@ -214,10 +214,15 @@ void MoveScreen(lv_disp_drv_t* disp_drv, int16_t height) {
     }
   }
   int16_t buffer_height = sdl_height - abs(height);
+  // The buffer only holds `buffer_height` valid rows. The area height MUST be
+  // buffer_height, not sdl_height: in the up case (y1 = 0) an sdl_height area
+  // makes monitor_flush read a full 240 rows from a pointer already advanced by
+  // sdl_width*abs(height), running sdl_width*abs(height) colors off the end of
+  // the stack array (a stack-buffer-overflow that corrupts adjacent frames).
   if (height >= 0) {
-    DrawBuffer(disp_drv, 0, height, sdl_width, sdl_height, (uint8_t*) color_p.data(), sdl_width * buffer_height * 2);
+    DrawBuffer(disp_drv, 0, height, sdl_width, buffer_height, (uint8_t*) color_p.data(), sdl_width * buffer_height * 2);
   } else {
-    DrawBuffer(disp_drv, 0, 0, sdl_width, sdl_height, (uint8_t*) (&color_p.at(sdl_width * abs(height))), sdl_width * buffer_height * 2);
+    DrawBuffer(disp_drv, 0, 0, sdl_width, buffer_height, (uint8_t*) (&color_p.at(sdl_width * abs(height))), sdl_width * buffer_height * 2);
   }
 }
 

sim/drivers/SpiNorFlash.cpp

@@ -38,10 +38,25 @@ void SpiNorFlash::Uninit() {
 }
 
 void SpiNorFlash::Sleep() {
+  sleeping = true;
   NRF_LOG_INFO("[SpiNorFlash] Sleep")
 }
 
-void SpiNorFlash::Wakeup() {NRF_LOG_INFO("[SpiNorFlash] Wakeup")}
+void SpiNorFlash::Wakeup() {
+  sleeping = false;
+  NRF_LOG_INFO("[SpiNorFlash] Wakeup")
+}
+
+void SpiNorFlash::AssertAwake(const char* op) const {
+  if (sleeping) {
+    // On hardware the chip is in deep power-down here: reads return garbage
+    // and writes are ignored. Fail deterministically in the sim instead of
+    // letting the corruption go unnoticed. abort() rather than throw: the
+    // caller is often littlefs C code, which exceptions must not unwind.
+    fprintf(stderr, "SpiNorFlash::%s while flash is asleep - firmware bug (missing wakeup)\n", op);
+    abort();
+  }
+}
 
 SpiNorFlash::Identification SpiNorFlash::ReadIdentification() {
   return {};
@@ -65,6 +80,7 @@ uint8_t SpiNorFlash::ReadConfigurationRegister() {
 
 void SpiNorFlash::Read(uint32_t address, uint8_t* buffer, size_t size) {
   static_assert(sizeof(uint8_t) == sizeof(char));
+  AssertAwake("Read");
   if (address + size * sizeof(uint8_t) > memorySize) {
     throw std::runtime_error("SpiNorFlash::Read out of bounds");
   }
@@ -76,6 +92,8 @@ void SpiNorFlash::WriteEnable() {
 }
 
 void SpiNorFlash::SectorErase(uint32_t sectorAddress) {
+  AssertAwake("SectorErase");
+  (void) sectorAddress;
 }
 
 uint8_t SpiNorFlash::ReadSecurityRegister() {
@@ -95,6 +113,7 @@ SpiNorFlash::Identification SpiNorFlash::GetIdentification() const {
 }
 
 void SpiNorFlash::Write(uint32_t address, const uint8_t* buffer, size_t size) {
+  AssertAwake("Write");
   if (address + size * sizeof(uint8_t) > memorySize) {
     throw std::runtime_error("SpiNorFlash::Write out of bounds");
   }

sim/drivers/SpiNorFlash.h

@@ -44,6 +44,7 @@ namespace Pinetime {
 
     private:
       Identification ReadIdentification();
+      void AssertAwake(const char* op) const;
 
       enum class Commands : uint8_t {
         PageProgram = 0x02,
@@ -64,6 +65,7 @@ namespace Pinetime {
 
       Identification device_id;
       std::fstream memoryFile;
+      bool sleeping = false;
     };
   }
 }

sim/host/ble_uuid.cpp

@@ -19,9 +19,33 @@
 
 #include "host/ble_uuid.h"
 
+#include <string.h>
 
+// sim: real comparison (the original stub returned 0 for every pair, which
+// breaks services that dispatch on the characteristic UUID).
 int
 ble_uuid_cmp(const ble_uuid_t *uuid1, const ble_uuid_t *uuid2)
 {
-    return 0;
+    if (uuid1->type != uuid2->type) {
+        return (int) uuid1->type - (int) uuid2->type;
+    }
+    switch (uuid1->type) {
+    case BLE_UUID_TYPE_16: {
+        const ble_uuid16_t *a = (const ble_uuid16_t *) uuid1;
+        const ble_uuid16_t *b = (const ble_uuid16_t *) uuid2;
+        return (int) a->value - (int) b->value;
+    }
+    case BLE_UUID_TYPE_32: {
+        const ble_uuid32_t *a = (const ble_uuid32_t *) uuid1;
+        const ble_uuid32_t *b = (const ble_uuid32_t *) uuid2;
+        return a->value == b->value ? 0 : (a->value < b->value ? -1 : 1);
+    }
+    case BLE_UUID_TYPE_128: {
+        const ble_uuid128_t *a = (const ble_uuid128_t *) uuid1;
+        const ble_uuid128_t *b = (const ble_uuid128_t *) uuid2;
+        return memcmp(a->value, b->value, sizeof(a->value));
+    }
+    default:
+        return -1;
+    }
 }

sim/host/os_mbuf.cpp

@@ -19,15 +19,40 @@
 
 #include "host/os_mbuf.h"
 
+#include <string.h>
+
+// sim: real single-buffer implementations (the original stubs were no-ops,
+// which silently broke any service using the proper NimBLE parsing idioms).
+// A fake mbuf is one flat buffer: om_data points at the data, om_len is its
+// length. There is no chaining.
 
 int
 os_mbuf_copydata(const struct os_mbuf *m, int off, int len, void *dst)
 {
+    if (m == NULL || m->om_data == NULL || off < 0 || len < 0 ||
+        off + len > m->om_len) {
+        return -1;
+    }
+    memcpy(dst, m->om_data + off, (size_t) len);
     return 0;
 }
 
+// Appends into the buffer om_data points at. For sim fake mbufs om_pkthdr_len
+// carries the destination capacity (bytes) so an over-long append is rejected
+// instead of overrunning the creator's stack buffer; a capacity of 0 means the
+// creator vouches for the buffer (e.g. write buffers, which are never
+// appended). Firmware code only checks the return value, exactly as it does
+// against the real NimBLE mbuf-pool exhaustion.
 int
 os_mbuf_append(struct os_mbuf *om, const void *data,  uint16_t len)
 {
+    if (om == NULL || om->om_data == NULL) {
+        return -1;
+    }
+    if (om->om_pkthdr_len != 0 && (uint32_t) om->om_len + len > om->om_pkthdr_len) {
+        return -1; // would overrun the response buffer
+    }
+    memcpy(om->om_data + om->om_len, data, len);
+    om->om_len += len;
     return 0;
-}
+}