Fix Updater potential overflow, add host tests #6954
Conversation
Fixes esp8266#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).
There was a problem hiding this comment.
(just test concerns)
Maybe use a different approach for test here? For example:
diff --git a/tests/host/Makefile b/tests/host/Makefile index 119e8f79..f6361f36 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)) @@ -236,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 sourcesNow we have FS... as addresses, without .ld
Values are from eagle.flash.16m15m.ld
| | ||
| #ifndef HOST_MOCK | ||
| wifi_set_sleep_type(NONE_SLEEP_T); | ||
| #endif |
There was a problem hiding this comment.
Can this be defined in the user_interface.cpp mock to avoid ifndef?
There was a problem hiding this comment.
It actually is defined there already, but unfortunately user_interface.cpp is not for CI testing but for building host apps and including that ends up eventually requiring the whole host app sources (with its own main, etc.).
The host app and host tests have differing build setups and assumptions (and goals, obviously). It's probably possible to refactor them completely to avoid the duplication here, but I'm not so sure this is the right time for it (3.0.0-ish feels right).
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.
4 participants
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).