From f8ab8b85f783bec9462f740e292f707b02222e68 Mon Sep 17 00:00:00 2001 From: James Foster Date: Thu, 19 Aug 2021 14:17:24 -0700 Subject: [PATCH 1/6] Rule of three: if you have a custom destructor, then you probably need a custom copy constructor and a custom copy assignment operator. --- SampleProjects/TestSomething/test/clientServer.cpp | 7 +++++++ cpp/arduino/Client.h | 14 ++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/SampleProjects/TestSomething/test/clientServer.cpp b/SampleProjects/TestSomething/test/clientServer.cpp index f088c821..5e66b6b3 100644 --- a/SampleProjects/TestSomething/test/clientServer.cpp +++ b/SampleProjects/TestSomething/test/clientServer.cpp @@ -19,6 +19,13 @@ unittest(Client) { assertEqual(outData + "\r\n", inData); } +unittest(Client_copy_constructor) { + Client client1; + Client client2; + client2 = client1; + assertTrue(true); +} + unittest(IPAddress) { IPAddress ipAddress0; assertEqual(0, ipAddress0.asWord()); diff --git a/cpp/arduino/Client.h b/cpp/arduino/Client.h index 154e618d..74d0809e 100644 --- a/cpp/arduino/Client.h +++ b/cpp/arduino/Client.h @@ -11,6 +11,20 @@ class Client : public Stream { mGodmodeDataIn = new String; } } + Client(const Client &client) { + // copy constructor + if (mGodmodeDataIn) { + mGodmodeDataIn = new String(mGodmodeDataIn->c_str()); + } + std::cout << __FILE__ << ":" << __LINE__ << std::endl; + } + Client & operator=(const Client &client) { + // copy assignment operator + if (mGodmodeDataIn) { + mGodmodeDataIn = new String(mGodmodeDataIn->c_str()); + } + return *this; + } ~Client() { if (mGodmodeDataIn) { delete mGodmodeDataIn; From 4be329e7373c3ee27c61f7392552795f533b554c Mon Sep 17 00:00:00 2001 From: James Foster Date: Thu, 19 Aug 2021 14:28:17 -0700 Subject: [PATCH 2/6] Remove debugging output. --- cpp/arduino/Client.h | 1 - 1 file changed, 1 deletion(-) diff --git a/cpp/arduino/Client.h b/cpp/arduino/Client.h index 74d0809e..ae368426 100644 --- a/cpp/arduino/Client.h +++ b/cpp/arduino/Client.h @@ -16,7 +16,6 @@ class Client : public Stream { if (mGodmodeDataIn) { mGodmodeDataIn = new String(mGodmodeDataIn->c_str()); } - std::cout << __FILE__ << ":" << __LINE__ << std::endl; } Client & operator=(const Client &client) { // copy assignment operator From a9ac410fd44bd554d2cf29ffc426a0e0fec02383 Mon Sep 17 00:00:00 2001 From: James Foster Date: Tue, 24 Aug 2021 20:52:17 -0700 Subject: [PATCH 3/6] Fix memory leak in copy constructor and copy assignment operator. --- cpp/arduino/Client.h | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/cpp/arduino/Client.h b/cpp/arduino/Client.h index ae368426..31290871 100644 --- a/cpp/arduino/Client.h +++ b/cpp/arduino/Client.h @@ -1,7 +1,7 @@ #pragma once -#include #include +#include class Client : public Stream { public: @@ -11,16 +11,20 @@ class Client : public Stream { mGodmodeDataIn = new String; } } - Client(const Client &client) { - // copy constructor - if (mGodmodeDataIn) { - mGodmodeDataIn = new String(mGodmodeDataIn->c_str()); + Client(const Client &client) { // copy constructor + if (this != &client) { // not a self-assignment + if (mGodmodeDataIn) { // replace what we previously had + delete mGodmodeDataIn; // get rid of previous value + mGodmodeDataIn = new String(client.mGodmodeDataIn->c_str()); + } } } - Client & operator=(const Client &client) { - // copy assignment operator - if (mGodmodeDataIn) { - mGodmodeDataIn = new String(mGodmodeDataIn->c_str()); + Client &operator=(const Client &client) { // copy assignment operator + if (this != &client) { // not a self-assignment + if (mGodmodeDataIn) { // replace what we previously had + delete mGodmodeDataIn; // get rid of previous value + mGodmodeDataIn = new String(client.mGodmodeDataIn->c_str()); + } } return *this; } From e323378e81a1e4407381cf3ca6a748d0069c3573 Mon Sep 17 00:00:00 2001 From: James Foster Date: Tue, 24 Aug 2021 21:00:43 -0700 Subject: [PATCH 4/6] Further work to address copy assignment memory leak. --- CHANGELOG.md | 1 + cpp/arduino/Client.h | 10 ++++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f29be1a6..caf736fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed - Change 266 files from CRLF to LF. +- Apply "rule of three" to Client copy constructor and copy assignment operator ### Deprecated diff --git a/cpp/arduino/Client.h b/cpp/arduino/Client.h index 31290871..759267a4 100644 --- a/cpp/arduino/Client.h +++ b/cpp/arduino/Client.h @@ -13,16 +13,18 @@ class Client : public Stream { } Client(const Client &client) { // copy constructor if (this != &client) { // not a self-assignment - if (mGodmodeDataIn) { // replace what we previously had - delete mGodmodeDataIn; // get rid of previous value + if (mGodmodeDataIn && + client.mGodmodeDataIn) { // replace what we previously had + delete mGodmodeDataIn; // get rid of previous value mGodmodeDataIn = new String(client.mGodmodeDataIn->c_str()); } } } Client &operator=(const Client &client) { // copy assignment operator if (this != &client) { // not a self-assignment - if (mGodmodeDataIn) { // replace what we previously had - delete mGodmodeDataIn; // get rid of previous value + if (mGodmodeDataIn && + client.mGodmodeDataIn) { // replace what we previously had + delete mGodmodeDataIn; // get rid of previous value mGodmodeDataIn = new String(client.mGodmodeDataIn->c_str()); } } From 2b7c6e377c776baab8add5d3a6fa54523d62f09a Mon Sep 17 00:00:00 2001 From: James Foster Date: Wed, 22 Sep 2021 15:10:19 -0700 Subject: [PATCH 5/6] Add comments describing test. --- SampleProjects/TestSomething/test/clientServer.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/SampleProjects/TestSomething/test/clientServer.cpp b/SampleProjects/TestSomething/test/clientServer.cpp index 5e66b6b3..dde26343 100644 --- a/SampleProjects/TestSomething/test/clientServer.cpp +++ b/SampleProjects/TestSomething/test/clientServer.cpp @@ -20,10 +20,13 @@ unittest(Client) { } unittest(Client_copy_constructor) { - Client client1; - Client client2; - client2 = client1; - assertTrue(true); + { // Client object contains a reference to a String object + Client c1; // Constructor instantiates a String (s1) + Client c2; // Constructor instantiates a String (s2) + c2 = c1; // Does c2 get a reference to s1 or a copy of it? + } // End of scope calls destructor on c1 and c2 + assertTrue(true); // Was s1 deleted once (with c1) or twice (also with c2)? + // Was s2 deleted at all (should be during assignment)? } unittest(IPAddress) { From 23c3670b394d5f4ab4ffd6d31649f86607b5a3d7 Mon Sep 17 00:00:00 2001 From: James Foster Date: Thu, 23 Sep 2021 08:53:44 -0700 Subject: [PATCH 6/6] The test is now more explicit about looking at the values expected. Before the fix to Client.h we get the following error: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ``` # Subtest: Client_copy_constructor ok 1 - assertEqual "1" == *(c1.mGodmodeDataIn) ok 2 - assertEqual "2" == *(c2.mGodmodeDataIn) not ok 3 - assertNotEqual c1.mGodmodeDataIn != c2.mGodmodeDataIn --- operator: != unwanted: 0x603000007750 actual: 0x603000007750 at: file: /Users/jfoster/Documents/Arduino/libraries/TestSomething/test/clientServer.cpp line: 32 ... ok 4 - assertEqual "1" == *(c1.mGodmodeDataIn) ok 5 - assertEqual "1" == *(c2.mGodmodeDataIn) not ok 6 - assertEqual "11" == *(c1.mGodmodeDataIn) --- operator: == expected: 11 actual: 112 at: file: /Users/jfoster/Documents/Arduino/libraries/TestSomething/test/clientServer.cpp line: 37 ... not ok 7 - assertEqual "12" == *(c2.mGodmodeDataIn) --- operator: == expected: 12 actual: 112 at: file: /Users/jfoster/Documents/Arduino/libraries/TestSomething/test/clientServer.cpp line: 38 ... AddressSanitizer:DEADLYSIGNAL ================================================================= ==16588==ERROR: AddressSanitizer: SEGV on unknown address (pc 0x0001076e5918 bp 0x7ffee8564290 sp 0x7ffee8564260 T0) ==16588==The signal is caused by a READ memory access. ==16588==Hint: this fault was caused by a dereference of a high value address (see register values below). Dissassemble the provided pc to learn which register was used. #0 0x1076e5918 in __asan::Allocator::Deallocate(void*, unsigned long, unsigned long, __sanitizer::BufferedStackTrace*, __asan::AllocType)+0x48 (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x5918) #1 0x107733125 in wrap__ZdlPv+0xe5 (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x53125) #2 0x10769d528 in String::~String() WString.h:65 #3 0x10769d228 in String::~String() WString.h:65 #4 0x1076a6856 in Client::~Client() Client.h:35 #5 0x1076a2f48 in Client::~Client() Client.h:33 #6 0x1076a3473 in test_Client_copy_constructor::task() clientServer.cpp:39 #7 0x1076a8390 in Test::test() ArduinoUnitTests.h:205 #8 0x1076a7f2d in Test::run(Test::ReporterTAP*) ArduinoUnitTests.h:176 #9 0x1076a5713 in Test::run_and_report(int, char**) ArduinoUnitTests.h:195 #10 0x1076a5658 in main clientServer.cpp:110 #11 0x7fff70a26cc8 in start+0x0 (libdyld.dylib:x86_64+0x1acc8) ==16588==Register values: rax = 0x0000000000000002 rbx = 0xbebebebebebebebe rcx = 0x0000000000000003 rdx = 0x0000000000000000 rdi = 0xbebebebebebebebe rsi = 0xbebebebebebebebe rbp = 0x00007ffee8564290 rsp = 0x00007ffee8564260 r8 = 0x00007ffee85642a0 r9 = 0x0000000000000002 r10 = 0xffffffffffffffff r11 = 0x00000fffffffffff r12 = 0x0000000000000002 r13 = 0x0000000000000000 r14 = 0x00007ffee85642a0 r15 = 0x0000000107780d40 AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x5918) in __asan::Allocator::Deallocate(void*, unsigned long, unsigned long, __sanitizer::BufferedStackTrace*, __asan::AllocType)+0x48 ==16588==ABORTING ...Unit testing clientServer.cpp with g++ for uno ✗ ``` After the fix we get the following: ``` # Subtest: Client_copy_constructor ok 1 - assertEqual "1" == *(c1.mGodmodeDataIn) ok 2 - assertEqual "2" == *(c2.mGodmodeDataIn) ok 3 - assertNotEqual c1.mGodmodeDataIn != c2.mGodmodeDataIn ok 4 - assertEqual "1" == *(c1.mGodmodeDataIn) ok 5 - assertEqual "1" == *(c2.mGodmodeDataIn) ok 6 - assertEqual "11" == *(c1.mGodmodeDataIn) ok 7 - assertEqual "12" == *(c2.mGodmodeDataIn) ok 8 - True true 1..8 ok 2 - Client_copy_constructor ``` --- .../TestSomething/test/clientServer.cpp | 27 ++++++++++++++----- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/SampleProjects/TestSomething/test/clientServer.cpp b/SampleProjects/TestSomething/test/clientServer.cpp index dde26343..7ffa2d6a 100644 --- a/SampleProjects/TestSomething/test/clientServer.cpp +++ b/SampleProjects/TestSomething/test/clientServer.cpp @@ -20,13 +20,26 @@ unittest(Client) { } unittest(Client_copy_constructor) { - { // Client object contains a reference to a String object - Client c1; // Constructor instantiates a String (s1) - Client c2; // Constructor instantiates a String (s2) - c2 = c1; // Does c2 get a reference to s1 or a copy of it? - } // End of scope calls destructor on c1 and c2 - assertTrue(true); // Was s1 deleted once (with c1) or twice (also with c2)? - // Was s2 deleted at all (should be during assignment)? + { // Client object contains a reference to a String object + Client c1; // Constructor instantiates a String (string1) + Client c2; // Constructor instantiates a String (string2) + c1.write('1'); + c2.write('2'); + assertEqual("1", *(c1.mGodmodeDataIn)); + assertEqual("2", *(c2.mGodmodeDataIn)); + c2 = c1; // c2 should get a copy of s1, not a reference to it + // and string2 should have been deleted during the assignment + assertNotEqual(c1.mGodmodeDataIn, c2.mGodmodeDataIn); + assertEqual("1", *(c1.mGodmodeDataIn)); + assertEqual("1", *(c2.mGodmodeDataIn)); + c1.write('1'); + c2.write('2'); + assertEqual("11", *(c1.mGodmodeDataIn)); + assertEqual("12", *(c2.mGodmodeDataIn)); + } // End of scope calls destructor on c1 and c2 + // Memory monitoring will give an error if delete is called twice on string1 + // The following assertion is just to confirm that we got through the above + assertTrue(true); } unittest(IPAddress) {