sharpdisplay: Fix memory corruption across soft-reset

jepler · jepler · commit d7c3f811afd9 · 2020-10-01T09:22:09.000-05:00
It was incorrect to NULL out the pointer to our heap allocated buffer in `reset`, because subsequent to framebuffer_reset, but while the heap was still active, we could call `get_bufinfo` again, leading to a fresh allocation on the heap that is about to be destroyed. Typical stack trace: ``` adafruit#1 0x0006c368 in sharpdisplay_framebuffer_get_bufinfo adafruit#2 0x0006ad6e in _refresh_display adafruit#3 0x0006b168 in framebufferio_framebufferdisplay_background adafruit#4 0x00069d22 in displayio_background adafruit#5 0x00045496 in supervisor_background_tasks adafruit#6 0x000446e8 in background_callback_run_all adafruit#7 0x00045546 in supervisor_run_background_tasks_if_tick adafruit#8 0x0005b042 in common_hal_neopixel_write adafruit#9 0x00044c4c in clear_temp_status adafruit#10 0x000497de in spi_flash_flush_keep_cache adafruit#11 0x00049a66 in supervisor_external_flash_flush adafruit#12 0x00044b22 in supervisor_flash_flush adafruit#13 0x0004490e in filesystem_flush adafruit#14 0x00043e18 in cleanup_after_vm adafruit#15 0x0004414c in run_repl adafruit#16 0x000441ce in main ``` When this happened -- which was inconsistent -- the display would keep some heap allocation across reset which is exactly what we need to avoid. NULLing the pointer in reconstruct follows what RGBMatrix does, and that code is a bit more battle-tested anyway. If I had a motivation for structuring the SharpMemory code differently, I can no longer recall it. Testing performed: Ran my complicated calculator program over multiple iterations without observing signs of heap corruption. Closes: adafruit#3473
diff --git a/shared-module/sharpdisplay/SharpMemoryFramebuffer.c b/shared-module/sharpdisplay/SharpMemoryFramebuffer.c
@@ -90,10 +90,6 @@ bool common_hal_sharpdisplay_framebuffer_get_pixels_in_byte_share_row(sharpdispl
 }
 
 void common_hal_sharpdisplay_framebuffer_reset(sharpdisplay_framebuffer_obj_t *self) {
-    if (!allocation_from_ptr(self->bufinfo.buf)) {
-        self->bufinfo.buf = NULL;
-    }
-
     if (self->bus != &self->inline_bus
 #if BOARD_SPI
         && self->bus != common_hal_board_get_spi()
@@ -105,7 +101,9 @@ void common_hal_sharpdisplay_framebuffer_reset(sharpdisplay_framebuffer_obj_t *s
 }
 
 void common_hal_sharpdisplay_framebuffer_reconstruct(sharpdisplay_framebuffer_obj_t *self) {
-
+    if (!allocation_from_ptr(self->bufinfo.buf)) {
+        self->bufinfo.buf = NULL;
+    }
 }
 
 void common_hal_sharpdisplay_framebuffer_get_bufinfo(sharpdisplay_framebuffer_obj_t *self, mp_buffer_info_t *bufinfo) {

Original file line number	Diff line number	Diff line change
`@@ -90,10 +90,6 @@ bool common_hal_sharpdisplay_framebuffer_get_pixels_in_byte_share_row(sharpdispl`
`90`	`90`	`}`
`91`	`91`
`92`	`92`	`void common_hal_sharpdisplay_framebuffer_reset(sharpdisplay_framebuffer_obj_t *self) {`
`93`		`- if (!allocation_from_ptr(self->bufinfo.buf)) {`
`94`		`- self->bufinfo.buf = NULL;`
`95`		`- }`
`96`		`-`
`97`	`93`	`if (self->bus != &self->inline_bus`
`98`	`94`	`#if BOARD_SPI`
`99`	`95`	`&& self->bus != common_hal_board_get_spi()`
`@@ -105,7 +101,9 @@ void common_hal_sharpdisplay_framebuffer_reset(sharpdisplay_framebuffer_obj_t *s`
`105`	`101`	`}`
`106`	`102`
`107`	`103`	`void common_hal_sharpdisplay_framebuffer_reconstruct(sharpdisplay_framebuffer_obj_t *self) {`
`108`		`-`
	`104`	`+ if (!allocation_from_ptr(self->bufinfo.buf)) {`
	`105`	`+ self->bufinfo.buf = NULL;`
	`106`	`+ }`
`109`	`107`	`}`
`110`	`108`
`111`	`109`	`void common_hal_sharpdisplay_framebuffer_get_bufinfo(sharpdisplay_framebuffer_obj_t self, mp_buffer_info_t bufinfo) {`