From a5520f8a3d7d866d4e97de8f04a08ab3dd908470 Mon Sep 17 00:00:00 2001 From: Scott Shawcroft Date: Wed, 3 Apr 2019 15:24:15 -0700 Subject: [PATCH 1/2] Set the terminal tilegrid NULL after free Without this, a double free can occur when a display (and terminal) is released and then a crash occurs. Upon a second release, different memory is released (sometimes the heap). When this is followed by an allocation for the flash cache, the cache can overwrite the active heap causing crashes. Fixes #1667 --- supervisor/shared/display.c | 1 + 1 file changed, 1 insertion(+) diff --git a/supervisor/shared/display.c b/supervisor/shared/display.c index e1400426e9..acf8e69d4f 100644 --- a/supervisor/shared/display.c +++ b/supervisor/shared/display.c @@ -81,6 +81,7 @@ void supervisor_start_terminal(uint16_t width_px, uint16_t height_px) { void supervisor_stop_terminal(void) { if (tilegrid_tiles != NULL) { free_memory(tilegrid_tiles); + tilegrid_tiles = NULL; supervisor_terminal_text_grid.inline_tiles = false; supervisor_terminal_text_grid.tiles = NULL; } From ceb6f2e4fc7ed80d6a7fe7966e973aa3715a4543 Mon Sep 17 00:00:00 2001 From: Scott Shawcroft Date: Wed, 3 Apr 2019 18:28:27 -0700 Subject: [PATCH 2/2] Rework flash flush so it preserves the cache This should make filesystem writes quicker and cause less heap churn. --- ports/atmel-samd/supervisor/internal_flash.c | 3 ++ ports/nrf/supervisor/internal_flash.c | 3 ++ supervisor/flash.h | 2 +- .../shared/external_flash/external_flash.c | 38 ++++++++++++------- supervisor/shared/filesystem.c | 6 ++- 5 files changed, 37 insertions(+), 15 deletions(-) diff --git a/ports/atmel-samd/supervisor/internal_flash.c b/ports/atmel-samd/supervisor/internal_flash.c index f1ceb5c927..6dd5ac0a70 100644 --- a/ports/atmel-samd/supervisor/internal_flash.c +++ b/ports/atmel-samd/supervisor/internal_flash.c @@ -76,6 +76,9 @@ uint32_t supervisor_flash_get_block_count(void) { void supervisor_flash_flush(void) { } +void supervisor_flash_release_cache(void) { +} + void flash_flush(void) { supervisor_flash_flush(); } diff --git a/ports/nrf/supervisor/internal_flash.c b/ports/nrf/supervisor/internal_flash.c index 2732132447..c18b708a3e 100644 --- a/ports/nrf/supervisor/internal_flash.c +++ b/ports/nrf/supervisor/internal_flash.c @@ -137,6 +137,9 @@ void supervisor_flash_flush(void) { _flash_page_addr = NO_CACHE; } +void supervisor_flash_release_cache(void) { +} + mp_uint_t supervisor_flash_read_blocks(uint8_t *dest, uint32_t block, uint32_t num_blocks) { // Must write out anything in cache before trying to read. supervisor_flash_flush(); diff --git a/supervisor/flash.h b/supervisor/flash.h index edf43f4b10..0a124353e6 100644 --- a/supervisor/flash.h +++ b/supervisor/flash.h @@ -40,7 +40,6 @@ void supervisor_flash_init(void); uint32_t supervisor_flash_get_block_size(void); uint32_t supervisor_flash_get_block_count(void); -void supervisor_flash_flush(void); // these return 0 on success, non-zero on error mp_uint_t supervisor_flash_read_blocks(uint8_t *dest, uint32_t block_num, uint32_t num_blocks); @@ -49,5 +48,6 @@ mp_uint_t supervisor_flash_write_blocks(const uint8_t *src, uint32_t block_num, struct _fs_user_mount_t; void supervisor_flash_init_vfs(struct _fs_user_mount_t *vfs); void supervisor_flash_flush(void); +void supervisor_flash_release_cache(void); #endif // MICROPY_INCLUDED_SUPERVISOR_FLASH_H diff --git a/supervisor/shared/external_flash/external_flash.c b/supervisor/shared/external_flash/external_flash.c index 73b7f7f911..ad10bab516 100644 --- a/supervisor/shared/external_flash/external_flash.c +++ b/supervisor/shared/external_flash/external_flash.c @@ -272,6 +272,9 @@ uint32_t supervisor_flash_get_block_count(void) { // Flush the cache that was written to the scratch portion of flash. Only used // when ram is tight. static bool flush_scratch_flash(void) { + if (current_sector == NO_SECTOR_LOADED) { + return true; + } // First, copy out any blocks that we haven't touched from the sector we've // cached. bool copy_to_scratch_ok = true; @@ -360,9 +363,25 @@ static bool allocate_ram_cache(void) { return success; } +static void release_ram_cache(void) { + if (supervisor_cache != NULL) { + free_memory(supervisor_cache); + supervisor_cache = NULL; + } else { + m_free(MP_STATE_VM(flash_ram_cache)); + } + MP_STATE_VM(flash_ram_cache) = NULL; +} + // Flush the cached sector from ram onto the flash. We'll free the cache unless // keep_cache is true. static bool flush_ram_cache(bool keep_cache) { + if (current_sector == NO_SECTOR_LOADED) { + if (!keep_cache) { + release_ram_cache(); + } + return true; + } // First, copy out any blocks that we haven't touched from the sector // we've cached. If we don't do this we'll erase the data during the sector // erase below. @@ -403,22 +422,13 @@ static bool flush_ram_cache(bool keep_cache) { } // We're done with the cache for now so give it back. if (!keep_cache) { - if (supervisor_cache != NULL) { - free_memory(supervisor_cache); - supervisor_cache = NULL; - } else { - m_free(MP_STATE_VM(flash_ram_cache)); - } - MP_STATE_VM(flash_ram_cache) = NULL; + release_ram_cache(); } return true; } // Delegates to the correct flash flush method depending on the existing cache. static void spi_flash_flush_keep_cache(bool keep_cache) { - if (current_sector == NO_SECTOR_LOADED) { - return; - } #ifdef MICROPY_HW_LED_MSC port_pin_set_output_level(MICROPY_HW_LED_MSC, true); #endif @@ -436,9 +446,11 @@ static void spi_flash_flush_keep_cache(bool keep_cache) { #endif } -// External flash function used. If called externally we assume we won't need -// the cache after. void supervisor_flash_flush(void) { + spi_flash_flush_keep_cache(true); +} + +void supervisor_flash_release_cache(void) { spi_flash_flush_keep_cache(false); } @@ -502,7 +514,7 @@ bool external_flash_write_block(const uint8_t *data, uint32_t block) { return write_flash(address, data, FILESYSTEM_BLOCK_SIZE); } if (current_sector != NO_SECTOR_LOADED) { - spi_flash_flush_keep_cache(true); + supervisor_flash_flush(); } if (MP_STATE_VM(flash_ram_cache) == NULL && !allocate_ram_cache()) { erase_sector(flash_device->total_size - SPI_FLASH_ERASE_SIZE); diff --git a/supervisor/shared/filesystem.c b/supervisor/shared/filesystem.c index 68c6f47499..fa34ed0dac 100644 --- a/supervisor/shared/filesystem.c +++ b/supervisor/shared/filesystem.c @@ -42,7 +42,9 @@ volatile bool filesystem_flush_requested = false; void filesystem_background(void) { if (filesystem_flush_requested) { - filesystem_flush(); + filesystem_flush_interval_ms = CIRCUITPY_FILESYSTEM_FLUSH_INTERVAL_MS; + // Flush but keep caches + supervisor_flash_flush(); filesystem_flush_requested = false; } } @@ -118,6 +120,8 @@ void filesystem_flush(void) { // Reset interval before next flush. filesystem_flush_interval_ms = CIRCUITPY_FILESYSTEM_FLUSH_INTERVAL_MS; supervisor_flash_flush(); + // Don't keep caches because this is called when starting or stopping the VM. + supervisor_flash_release_cache(); } void filesystem_set_internal_writable_by_usb(bool writable) {