From 5bc3079217ba91cab2f215750363c7f34e735fac Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Wed, 8 Jan 2020 17:38:28 -0800 Subject: [PATCH] Fix Updater potential overflow, add host tests (#6954) * Fix Updater potential overflow, add host tests Fixes #4674 The Updater class could, when exactly 4K bytes were in the buffer but not yet written to flash, allow overwriting data written to it beyond the passed-in size parameter. Fix per @jason-but's suggestion, and add a host test (plus minor changes to Updater code to support host testing). * Add missed mock file * Remove most testing ifdefs fro updater Per @mcspr's suggestion, we can pass in fake link symbols allowing Updater to take the address of `_FS_start`/etc. even when building on the host for testing. There is still a single remaining wifi_set_power_mode ifdef'd and a duplication of the digitalWrite/pinMode for testing vs. host building. Co-authored-by: Develo --- cores/esp8266/Updater.cpp | 6 ++-- tests/host/Makefile | 8 +++-- tests/host/common/MockDigital.cpp | 56 +++++++++++++++++++++++++++++++ tests/host/core/test_Updater.cpp | 56 +++++++++++++++++++++++++++++++ 4 files changed, 121 insertions(+), 5 deletions(-) create mode 100644 tests/host/common/MockDigital.cpp create mode 100644 tests/host/core/test_Updater.cpp diff --git a/cores/esp8266/Updater.cpp b/cores/esp8266/Updater.cpp index ebb893aabd..70d16a55ed 100644 --- a/cores/esp8266/Updater.cpp +++ b/cores/esp8266/Updater.cpp @@ -104,7 +104,9 @@ bool UpdaterClass::begin(size_t size, int command, int ledPin, uint8_t ledOn) { _reset(); clearError(); // _error = 0 +#ifndef HOST_MOCK wifi_set_sleep_type(NONE_SLEEP_T); +#endif //address where we will start writing the update uintptr_t updateStartAddress = 0; @@ -378,9 +380,7 @@ size_t UpdaterClass::write(uint8_t *data, size_t len) { if(hasError() || !isRunning()) return 0; - if(len > remaining()){ - //len = remaining(); - //fail instead + if(progress() + _bufferLen + len > _size) { _setError(UPDATE_ERROR_SPACE); return 0; } diff --git a/tests/host/Makefile b/tests/host/Makefile index aaf8a88d91..f6361f36aa 100644 --- a/tests/host/Makefile +++ b/tests/host/Makefile @@ -6,6 +6,7 @@ LIBRARIES_PATH := ../../libraries FORCE32 ?= 1 OPTZ ?= -Os V ?= 0 +DEFSYM_FS ?= -Wl,--defsym,_FS_start=0x40300000 -Wl,--defsym,_FS_end=0x411FA000 -Wl,--defsym,_FS_page=0x100 -Wl,--defsym,_FS_block=0x2000 -Wl,--defsym,_EEPROM_start=0x411fb000 MAKEFILE = $(word 1, $(MAKEFILE_LIST)) @@ -70,6 +71,7 @@ CORE_CPP_FILES := $(addprefix $(CORE_PATH)/,\ Schedule.cpp \ HardwareSerial.cpp \ crc32.cpp \ + Updater.cpp \ ) \ $(addprefix $(LIBRARIES_PATH)/ESP8266SdFat/src/, \ FatLib/FatFile.cpp \ @@ -98,6 +100,7 @@ MOCK_CPP_FILES_COMMON := $(addprefix common/,\ MockUART.cpp \ MockTools.cpp \ MocklwIP.cpp \ + MockDigital.cpp \ ) MOCK_CPP_FILES := $(MOCK_CPP_FILES_COMMON) $(addprefix common/,\ @@ -136,7 +139,8 @@ TEST_CPP_FILES := \ core/test_md5builder.cpp \ core/test_string.cpp \ core/test_PolledTimeout.cpp \ - core/test_Print.cpp + core/test_Print.cpp \ + core/test_Updater.cpp PREINCLUDES := \ -include common/mock.h \ @@ -233,7 +237,7 @@ $(BINDIR)/core.a: $(C_OBJECTS) $(CPP_OBJECTS_CORE) ranlib -c $@ $(OUTPUT_BINARY): $(CPP_OBJECTS_TESTS) $(BINDIR)/core.a - $(VERBLD) $(CXX) $(LDFLAGS) $^ -o $@ + $(VERBLD) $(CXX) $(DEFSYM_FS) $(LDFLAGS) $^ -o $@ ################################################# # building ino sources diff --git a/tests/host/common/MockDigital.cpp b/tests/host/common/MockDigital.cpp new file mode 100644 index 0000000000..aa04527ab5 --- /dev/null +++ b/tests/host/common/MockDigital.cpp @@ -0,0 +1,56 @@ +/* + digital.c - wiring digital implementation for esp8266 + + Copyright (c) 2015 Hristo Gochkov. All rights reserved. + This file is part of the esp8266 core for Arduino environment. + + This library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + This library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with this library; if not, write to the Free Software + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +*/ +#define ARDUINO_MAIN +#include "wiring_private.h" +#include "pins_arduino.h" +#include "c_types.h" +#include "eagle_soc.h" +#include "ets_sys.h" +#include "user_interface.h" +#include "core_esp8266_waveform.h" +#include "interrupts.h" + +extern "C" { + +static uint8_t _mode[17]; +static uint8_t _gpio[17]; + +extern void pinMode(uint8_t pin, uint8_t mode) { + if (pin < 17) { + _mode[pin] = mode; + } +} + +extern void digitalWrite(uint8_t pin, uint8_t val) { + if (pin < 17) { + _gpio[pin] = val; + } +} + +extern int digitalRead(uint8_t pin) { + if (pin < 17) { + return _gpio[pin] != 0; + } else { + return 0; + } +} + +}; diff --git a/tests/host/core/test_Updater.cpp b/tests/host/core/test_Updater.cpp new file mode 100644 index 0000000000..f3b850e130 --- /dev/null +++ b/tests/host/core/test_Updater.cpp @@ -0,0 +1,56 @@ +/* + test_Updater.cpp - Updater tests + Copyright © 2019 Earle F. Philhower, III + + Permission is hereby granted, free of charge, to any person obtaining a copy + of this software and associated documentation files (the "Software"), to deal + in the Software without restriction, including without limitation the rights + to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + copies of the Software, and to permit persons to whom the Software is + furnished to do so, subject to the following conditions: + + The above copyright notice and this permission notice shall be included in + all copies or substantial portions of the Software. + */ + +#include +#include + + +// Use a SPIFFS file because we can't instantiate a virtual class like Print +TEST_CASE("Updater fails when writes overflow requested size", "[core][Updater]") +{ + UpdaterClass *u; + uint8_t buff[6000]; + memset(buff, 0, sizeof(buff)); + u = new UpdaterClass(); + REQUIRE(u->begin(6000)); + REQUIRE(u->write(buff, 1000)); + REQUIRE(u->write(buff, 1000)); + REQUIRE(u->write(buff, 1000)); + REQUIRE(u->write(buff, 1000)); + REQUIRE(u->write(buff, 1000)); + REQUIRE(u->write(buff, 1000)); + REQUIRE(u->remaining() == 0); + // All bytes written, should fail next + REQUIRE(!u->write(buff, 1000)); + delete u; + + // Updater to a 4K aligned size, check nomissing over/underflow + u = new UpdaterClass(); + REQUIRE(u->begin(8192)); + REQUIRE(u->remaining() == 8192); + REQUIRE(u->write(buff, 4096)); + REQUIRE(u->write(buff, 4096)); + REQUIRE(!u->write(buff, 1)); + delete u; + + // Issue #4674 + u = new UpdaterClass(); + REQUIRE(u->begin(5000)); + REQUIRE(u->write(buff, 2048)); + REQUIRE(u->write(buff, 2048)); + // Should fail, would write 6KB + REQUIRE(!u->write(buff, 2048)); + delete u; +}