From 330735625e90fa962f61e4583314260824f198fc Mon Sep 17 00:00:00 2001 From: Andrea Gilardoni Date: Wed, 19 Feb 2025 11:10:41 +0100 Subject: [PATCH 1/2] Added unit test for virtual destructors The purpose of this tests is to hihglight the need for a virtual destructor through valgrind unit test execution --- test/CMakeLists.txt | 1 + test/include/TCPClientMock.h | 45 +++++++++++++++ .../src/Interfaces/test_virtualDestructor.cpp | 36 ++++++++++++ virtual_destructors_testing/benchmark.sh | 24 ++++++++ .../virtual_destructors_testing.ino | 55 +++++++++++++++++++ 5 files changed, 161 insertions(+) create mode 100644 test/include/TCPClientMock.h create mode 100644 test/src/Interfaces/test_virtualDestructor.cpp create mode 100755 virtual_destructors_testing/benchmark.sh create mode 100644 virtual_destructors_testing/virtual_destructors_testing.ino diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index d8050249..0afe4729 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -112,6 +112,7 @@ set(TEST_SRCS src/WCharacter/test_isUpperCase.cpp src/WCharacter/test_isWhitespace.cpp src/WCharacter/test_toAscii.cpp + src/Interfaces/test_virtualDestructor.cpp ) set(TEST_DUT_SRCS diff --git a/test/include/TCPClientMock.h b/test/include/TCPClientMock.h new file mode 100644 index 00000000..91f5c886 --- /dev/null +++ b/test/include/TCPClientMock.h @@ -0,0 +1,45 @@ +/* + * Copyright (c) 2020 Arduino. All rights reserved. + * + * SPDX-License-Identifier: LGPL-2.1-or-later + */ + +#ifndef TCPCLIENT_MOCK_H_ +#define TCPCLIENT_MOCK_H_ + +/************************************************************************************** + * INCLUDE + **************************************************************************************/ + +#include + +/************************************************************************************** + * CLASS DECLARATION + **************************************************************************************/ + +/* + * The purpose of this class is currently to highlight the effects of lacking virtual destructor + */ + +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wunused-parameter" + +class TCPClientMock : public Client { +public: + virtual int connect(IPAddress ip, uint16_t port) { return 0; } + virtual int connect(const char *host, uint16_t port) { return 0; } + virtual size_t write(uint8_t) { return 0; } + virtual size_t write(const uint8_t *buf, size_t size) { return 0;} + virtual int available() { return 0; } + virtual int read() { return 0; } + virtual int read(uint8_t *buf, size_t size) { return 0;} + virtual int peek() { return 0; } + virtual void flush() {} + virtual void stop() {} + virtual uint8_t connected() { return 0;} + virtual operator bool() { return true; } +}; +#pragma GCC diagnostic pop + + +#endif /* TCPCLIENT_MOCK_H_ */ diff --git a/test/src/Interfaces/test_virtualDestructor.cpp b/test/src/Interfaces/test_virtualDestructor.cpp new file mode 100644 index 00000000..decc8cb4 --- /dev/null +++ b/test/src/Interfaces/test_virtualDestructor.cpp @@ -0,0 +1,36 @@ +/* + * Copyright (c) 2020 Arduino. All rights reserved. + * + * SPDX-License-Identifier: LGPL-2.1-or-later + */ + +#include +#include +#include + +/* + * The purpose of these tests is to highlight potential memory leaking + * issues that may arise from the lack of virtual destructors. + * These test cases will never fail under unit testing, + * but they should trigger valgrind error reporting + */ + +TEST_CASE("Testing polymorphic IPAddress memory free", "[ipaddress-delete-01]") +{ + arduino::Printable* p = new IPAddress(); + +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdelete-non-virtual-dtor" + delete p; +#pragma GCC diagnostic pop +} + +TEST_CASE("Testing polymorphic client memory free", "[client-delete-01]") +{ + arduino::Client* p = new TCPClientMock; + +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdelete-non-virtual-dtor" + delete p; +#pragma GCC diagnostic pop +} diff --git a/virtual_destructors_testing/benchmark.sh b/virtual_destructors_testing/benchmark.sh new file mode 100755 index 00000000..0d8a01f3 --- /dev/null +++ b/virtual_destructors_testing/benchmark.sh @@ -0,0 +1,24 @@ +#!/bin/bash + +FQBNS=( + arduino-git:renesas_portenta_main:unor4wifi + # arduino-git:mbed:envie_m7 + # arduino-git:avr:uno + # arduino-git:samd:mkrwifi1010 + # arduino-git:mbed:opta +) + +branches=( + master + virtual-destructors +) + +for branch in "${branches[@]}"; do + git checkout $branch + for fqbn in $"${FQBNS[@]}"; do + echo "compiling for " $fqbn.$branch + # arduino-cli compile -b $fqbn | tee $fqbn.$branch.log + arduino-cli compile -b $fqbn --build-property "compiler.cpp.extra_flags=\"-fdump-record-layouts\"" + # /home/agilardoni/.arduino15/packages/arduino/tools/arm-none-eabi-gcc/7-2017q4/bin/arm-none-eabi-size -A /tmp/arduino/sketches/97A7C8915D2115841AF9E1023FFD9539/virtual_destructors_testing.ino.elf | tee $fqbn.$branch.size.log + done +done diff --git a/virtual_destructors_testing/virtual_destructors_testing.ino b/virtual_destructors_testing/virtual_destructors_testing.ino new file mode 100644 index 00000000..6fb0c0db --- /dev/null +++ b/virtual_destructors_testing/virtual_destructors_testing.ino @@ -0,0 +1,55 @@ + +#if defined(ARDUINO_SAMD_MKR1000) + #include + #include +#elif defined(ARDUINO_SAMD_MKRWIFI1010) || defined(ARDUINO_SAMD_NANO_33_IOT) || defined(ARDUINO_AVR_UNO_WIFI_REV2) || defined (ARDUINO_NANO_RP2040_CONNECT) + #include + #include +#elif defined(ARDUINO_PORTENTA_H7_M7) || defined(ARDUINO_PORTENTA_H7_M7) || \ + defined(ARDUINO_NICLA_VISION) || defined(ARDUINO_OPTA) || defined(ARDUINO_GIGA) + #include + #include +#elif defined(ARDUINO_PORTENTA_C33) + #include + #include +#elif defined(ARDUINO_ARCH_ESP8266) + #include + #include +#elif defined(ARDUINO_ARCH_ESP32) + #include + #include +#elif defined(ARDUINO_UNOR4_WIFI) + #include +#elif defined(ARDUINO_RASPBERRY_PI_PICO_W) + #include + #include +#endif +void setup() { + Serial.begin(115200); + + while(!Serial); + + Serial.println("Hello world"); + +#if defined(ARDUINO_SAMD_MKR1000) || \ + defined(ARDUINO_SAMD_MKRWIFI1010) || \ + defined(ARDUINO_SAMD_NANO_33_IOT) || \ + defined(ARDUINO_AVR_UNO_WIFI_REV2) || \ + defined(ARDUINO_NANO_RP2040_CONNECT) || \ + defined(ARDUINO_PORTENTA_H7_M7) || \ + defined(ARDUINO_PORTENTA_H7_M7) || \ + defined(ARDUINO_NICLA_VISION) || \ + defined(ARDUINO_OPTA) || \ + defined(ARDUINO_GIGA) || \ + defined(ARDUINO_PORTENTA_C33) || \ + defined(ARDUINO_ARCH_ESP8266) || \ + defined(ARDUINO_ARCH_ESP32) || \ + defined(ARDUINO_UNOR4_WIFI) || \ + defined(ARDUINO_RASPBERRY_PI_PICO_W) + Client *c = new WiFiClient(); // sizeof // as global variable + delete c; +#endif +} + +void loop() { +} From 9dab7bb4c753597f143e5469b070ba1a12c60fcc Mon Sep 17 00:00:00 2001 From: Andrea Gilardoni Date: Wed, 27 Sep 2023 10:01:50 +0200 Subject: [PATCH 2/2] Added missing virtual destructors --- api/Print.h | 2 ++ api/Printable.h | 1 + 2 files changed, 3 insertions(+) diff --git a/api/Print.h b/api/Print.h index 2016d7d5..ce023b6a 100644 --- a/api/Print.h +++ b/api/Print.h @@ -43,6 +43,8 @@ class Print void setWriteError(int err = 1) { write_error = err; } public: Print() : write_error(0) {} + virtual ~Print() {} + int getWriteError() { return write_error; } void clearWriteError() { setWriteError(0); } diff --git a/api/Printable.h b/api/Printable.h index 850c8d21..c14761f2 100644 --- a/api/Printable.h +++ b/api/Printable.h @@ -34,6 +34,7 @@ class Print; class Printable { public: + virtual ~Printable() {} virtual size_t printTo(Print& p) const = 0; };