From d7c3f811afd924bae8e0b009a4cfadd6fab29584 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Thu, 1 Oct 2020 09:21:39 -0500 Subject: sharpdisplay: Fix memory corruption across soft-reset 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: ``` #1 0x0006c368 in sharpdisplay_framebuffer_get_bufinfo #2 0x0006ad6e in _refresh_display #3 0x0006b168 in framebufferio_framebufferdisplay_background #4 0x00069d22 in displayio_background #5 0x00045496 in supervisor_background_tasks #6 0x000446e8 in background_callback_run_all #7 0x00045546 in supervisor_run_background_tasks_if_tick #8 0x0005b042 in common_hal_neopixel_write #9 0x00044c4c in clear_temp_status #10 0x000497de in spi_flash_flush_keep_cache #11 0x00049a66 in supervisor_external_flash_flush #12 0x00044b22 in supervisor_flash_flush #13 0x0004490e in filesystem_flush #14 0x00043e18 in cleanup_after_vm #15 0x0004414c in run_repl #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: #3473 --- shared-module/sharpdisplay/SharpMemoryFramebuffer.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) (limited to 'shared-module/sharpdisplay') diff --git a/shared-module/sharpdisplay/SharpMemoryFramebuffer.c b/shared-module/sharpdisplay/SharpMemoryFramebuffer.c index 71e00d7f0..c23446828 100644 --- 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) { -- cgit v1.2.3 From b520498c733356afdc418bd5a9841c7496a3926e Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Thu, 1 Oct 2020 10:30:35 -0500 Subject: sharpdisplay: Re-use supervisor allocations if possible This is enabled by #3482 I was unable to determine why previously I had added sizeof(void*) to the GC heap allocation, so I removed that code as a mistake. --- shared-module/sharpdisplay/SharpMemoryFramebuffer.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) (limited to 'shared-module/sharpdisplay') diff --git a/shared-module/sharpdisplay/SharpMemoryFramebuffer.c b/shared-module/sharpdisplay/SharpMemoryFramebuffer.c index c23446828..b199e98d6 100644 --- a/shared-module/sharpdisplay/SharpMemoryFramebuffer.c +++ b/shared-module/sharpdisplay/SharpMemoryFramebuffer.c @@ -34,21 +34,22 @@ #include "shared-module/sharpdisplay/SharpMemoryFramebuffer.h" #include "supervisor/memory.h" +#include "supervisor/shared/safe_mode.h" #define SHARPMEM_BIT_WRITECMD_LSB (0x80) #define SHARPMEM_BIT_VCOM_LSB (0x40) -static inline void *hybrid_alloc(size_t sz) { - if (gc_alloc_possible()) { - return m_malloc(sz + sizeof(void*), true); - } else { - supervisor_allocation *allocation = allocate_memory(align32_size(sz), false); - if (!allocation) { - return NULL; - } +static void *hybrid_alloc(size_t sz) { + supervisor_allocation *allocation = allocate_memory(align32_size(sz), false); + if (allocation) { memset(allocation->ptr, 0, sz); return allocation->ptr; } + if (gc_alloc_possible()) { + return m_malloc(sz, true); + } + reset_into_safe_mode(MEM_MANAGE); + return NULL; // unreached } static inline void hybrid_free(void *ptr_in) { @@ -155,7 +156,8 @@ void common_hal_sharpdisplay_framebuffer_construct(sharpdisplay_framebuffer_obj_ int row_stride = common_hal_sharpdisplay_framebuffer_get_row_stride(self); self->bufinfo.len = row_stride * height + 2; - self->bufinfo.buf = gc_alloc(self->bufinfo.len, false, true); + // re-use a supervisor allocation if possible + self->bufinfo.buf = hybrid_alloc(self->bufinfo.len); uint8_t *data = self->bufinfo.buf; *data++ = SHARPMEM_BIT_WRITECMD_LSB; -- cgit v1.2.3