Fix random crash in unit test (#12)

This was caused by my stupidity to not correctly use the C API. I didn't call `cppIni_close` before leaving the test cases. This caused some dangling pointers and open file handles ultimately resulting in a crash of the test executable.
Master92 · Nov 14, 2024 · 2d5b539 · 2d5b539
2 parents daeb61d + 2607aa2
commit 2d5b539
Show file tree

Hide file tree

Showing 6 changed files with 122 additions and 80 deletions.
diff --git a/include/cppIni/cppIni_c.h b/include/cppIni/cppIni_c.h
@@ -23,7 +23,6 @@
 extern "C" {
 #endif
 
-typedef void* pFile;
 /// \file cppIni_c.h
 /// \brief The C API for cppIni
 ///
@@ -32,12 +31,14 @@ typedef void* pFile;
 /// the C++ API. It is intended for use with languages that do not support
 /// C++.
 
-CPPINI_EXPORT pFile cppIni_open(const char* filename); ///< Opens a file
-CPPINI_EXPORT void cppIni_close(pFile* file); ///< Closes a file
-CPPINI_EXPORT void cppIni_set(pFile file, const char* section, const char* key, const char* value); ///< Sets a value
-CPPINI_EXPORT const char* cppIni_gets(pFile file, const char* section, const char* key, char* out, size_t outSize); ///< Gets a string
-CPPINI_EXPORT int cppIni_geti(pFile file, const char* section, const char* key); ///< Gets an integer
-CPPINI_EXPORT float cppIni_getf(pFile file, const char* section, const char* key); ///< Gets a float
+CPPINI_EXPORT void* cppIni_open(const char* filename); ///< Opens a file
+CPPINI_EXPORT void cppIni_close(void** file); ///< Closes a file
+
+CPPINI_EXPORT void cppIni_set(void* file, const char* section, const char* key, const char* value); ///< Sets a value
+
+CPPINI_EXPORT const char* cppIni_gets(const void* file, const char* section, const char* key, char* out, size_t outSize); ///< Gets a string
+CPPINI_EXPORT int cppIni_geti(const void* file, const char* section, const char* key); ///< Gets an integer
+CPPINI_EXPORT float cppIni_getf(const void* file, const char* section, const char* key); ///< Gets a float
 
 #ifdef __cplusplus
 }

diff --git a/src/CInterface.cpp b/src/CInterface.cpp
@@ -24,14 +24,14 @@
 ///
 /// \param[in] filename The name of the file to open
 /// \return A pointer to a File object
-pFile cppIni_open(const char* filename)
+void* cppIni_open(const char* filename)
 {
     return new File(filename);
 }
 
 /// Closes a file that was opened with cppIni_open().
 /// \param[in] file A pointer to a File object
-void cppIni_close(pFile* file)
+void cppIni_close(void** const file)
 {
     delete static_cast<File*>(*file);
 }
@@ -40,7 +40,7 @@ void cppIni_close(pFile* file)
 /// \param[in] section The name of the section to add
 /// \param[in] key The name of the key to add
 /// \param[in] value The value to add
-void cppIni_set(pFile file, const char* section, const char* key, const char* value)
+void cppIni_set(void* const file, const char* const section, const char* const key, const char* value)
 {
     static_cast<File*>(file)->set(section, key, value);
 }
@@ -51,9 +51,9 @@ void cppIni_set(pFile file, const char* section, const char* key, const char* va
 /// \param[out] out A buffer to store the value in
 /// \param[in] outSize The size of the buffer
 /// \return A pointer to the buffer
-const char* cppIni_gets(pFile file, const char* section, const char* key, char* out, size_t outSize)
+const char* cppIni_gets(const void* const file, const char* const section, const char* const key, char* out, size_t outSize)
 {
-    const auto value = static_cast<File*>(file)->get<std::string>(section, key);
+    const auto value = static_cast<const File*>(file)->get<std::string>(section, key);
 
     if (value.empty()) {
         return out;
@@ -68,17 +68,17 @@ const char* cppIni_gets(pFile file, const char* section, const char* key, char*
 /// \param[in] section The name of the section to get
 /// \param[in] key The name of the key to get
 /// \return The value of the key
-int cppIni_geti(pFile file, const char* section, const char* key)
+int cppIni_geti(const void* const file, const char* const section, const char* const key)
 {
-    return static_cast<File*>(file)->get<int>(section, key);
+    return static_cast<const File*>(file)->get<int>(section, key);
 }
 
 /// \see cppIni_gets
 /// \param[in] file A pointer to a File object
 /// \param[in] section The name of the section to get
 /// \param[in] key The name of the key to get
 /// \return The value of the key
-float cppIni_getf(pFile file, const char* section, const char* key)
+float cppIni_getf(const void* const file, const char* const section, const char* const key)
 {
-    return static_cast<File*>(file)->get<float>(section, key);
+    return static_cast<const File*>(file)->get<float>(section, key);
 }
diff --git a/tests/CInterfaceTest.cpp b/tests/CInterfaceTest.cpp
@@ -1,25 +1,28 @@
-// cppIni - A C++20 library for reading and writing INI files
-// Copyright (C) 2023-2024 Nils Hofmann <[email protected]>
-//
-// This program is free software: you can redistribute it and/or modify
-// it under the terms of the GNU General Public License as published by
-// the Free Software Foundation, either version 3 of the License, or
-// (at your option) any later version.
-//
-// This program 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 General Public License for more details.
-//
-// You should have received a copy of the GNU General Public License
-// along with this program.  If not, see <https://www.gnu.org/licenses/>.
+/*
+ * cppIni - A C++20 library for reading and writing INI files
+ * Copyright (C) 2023-2024 Nils Hofmann <[email protected]>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
 
 #include <array>
 #include <filesystem>
 #include <format>
 
 #include <cppIni/cppIni_c.h>
 #include <doctest/doctest.h>
+#include "utils.h"
 
 static const std::string fileName = std::format("{}{}", WORKING_DIR, "/res/test.ini");;
 
@@ -33,67 +36,50 @@ TEST_CASE("Construction of File object")
     CHECK_NOTHROW(cppIni_close(&file));
 }
 
-struct ScopeGuard
-{
-    ScopeGuard(pFile file) : file(file){};
-    ~ScopeGuard(){ cppIni_close(&file); };
-    pFile file;
-};
-
 TEST_CASE("Read a string entry")
 {
-    void* file = cppIni_open(fileName.c_str());
-    ScopeGuard guard{file};
+    auto file = utils::ScopeGuard<void*, cppIni_close>(cppIni_open(fileName.c_str()));
 
     std::array<char, 64> buffer{0};
-    cppIni_gets(file, "Section1", "Entry1", buffer.data(), buffer.size());
+    cppIni_gets(*file, "Section1", "Entry1", buffer.data(), buffer.size());
 
     CHECK_EQ(std::string_view{buffer.data()}, "Value1");
 }
 
 TEST_CASE("Try to read a non-existing entry")
 {
-    void* file = cppIni_open(fileName.c_str());
-    ScopeGuard guard{file};
+    auto file = utils::ScopeGuard<void*, cppIni_close>(cppIni_open(fileName.c_str()));
 
     std::array<char, 64> buffer{0};
-    CHECK_EQ(cppIni_gets(file, "Section1", "NonExistingEntry", buffer.data(), buffer.size()), buffer.data());
+    CHECK_EQ(cppIni_gets(*file, "Section1", "NonExistingEntry", buffer.data(), buffer.size()), buffer.data());
     CHECK_EQ(buffer[0], '\0');
 }
 
 TEST_CASE("Change a value")
 {
-    constexpr auto tempFileName = "tmp.ini";
-    std::filesystem::copy_file(fileName, tempFileName);
-
-    {
-        constexpr auto newValue = 1337;
-        void* file = cppIni_open(tempFileName);
-        ScopeGuard guard{file};
+    constexpr auto newValue = 1337;
 
-        const auto previousValue = cppIni_geti(file, "Section1", "IntEntry");
-        CHECK_NE(previousValue, newValue);
-        cppIni_set(file, "Section1", "IntEntry", std::to_string(newValue).c_str());
-        CHECK_EQ(cppIni_geti(file, "Section1", "IntEntry"), newValue);
-    }
+    utils::TempFile tmpFile(fileName);
+    auto file = utils::ScopeGuard<void*, cppIni_close>(cppIni_open(tmpFile.filename().data()));
 
-    std::filesystem::remove(tempFileName);
+    const auto previousValue = cppIni_geti(*file, "Section1", "IntEntry");
+    CHECK_NE(previousValue, newValue);
+    cppIni_set(*file, "Section1", "IntEntry", std::to_string(newValue).c_str());
+    CHECK_EQ(cppIni_geti(*file, "Section1", "IntEntry"), newValue);
 }
 
 TEST_CASE("Read an integer entry")
 {
-    void* file = cppIni_open(fileName.c_str());
-    ScopeGuard guard{file};
+    auto file = utils::ScopeGuard<void*, cppIni_close>(cppIni_open(fileName.c_str()));
 
-    CHECK_EQ(cppIni_geti(file, "Section1", "IntEntry"), 42);
+    CHECK_EQ(cppIni_geti(*file, "Section1", "IntEntry"), 42);
 }
 
 TEST_CASE("Read a floating point value entry")
 {
-    void* file = cppIni_open(fileName.c_str());
-    ScopeGuard guard{file};
+    auto file = utils::ScopeGuard<void*, cppIni_close>(cppIni_open(fileName.c_str()));
 
-    CHECK_LT(std::abs(cppIni_getf(file, "Section1.Subsection1", "DoubleEntry") - 3.1415), 0.001);
+    CHECK_LT(std::abs(cppIni_getf(*file, "Section1.Subsection1", "DoubleEntry") - 3.1415), 0.001);
 }
 
 TEST_SUITE_END();
diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
@@ -7,6 +7,7 @@ set(TEST_SOURCES
     FileTest.cpp
     SectionTest.cpp
     CInterfaceTest.cpp
+    utils.h
 )
 
 add_executable(${PROJECT_NAME}_tests ${TEST_SOURCES})

diff --git a/tests/FileTest.cpp b/tests/FileTest.cpp
@@ -1,6 +1,6 @@
 /*
  * cppIni - A C++20 library for reading and writing INI files
- * Copyright (C) 2023 Nils Hofmann <[email protected]>
+ * Copyright (C) 2023-2024 Nils Hofmann <[email protected]>
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -21,26 +21,14 @@
 #include <doctest/doctest.h>
 
 #include <cppIni/File.h>
+#include "utils.h"
 
 using namespace std::literals;
 
 static const std::string fileName = std::format("{}{}", WORKING_DIR, "/res/test.ini");
 
 TEST_SUITE_BEGIN("File");
 
-class FileFixture
-{
-public:
-    FileFixture() {
-        std::filesystem::copy_file(::fileName, fileName);
-    }
-    ~FileFixture() {
-        std::filesystem::remove(fileName);
-    }
-protected:
-    const std::string fileName = std::format("{}{}", WORKING_DIR, "/res/tmp.ini");
-};
-
 TEST_CASE("Failing construction of an empty File object")
 {
     CHECK_THROWS(File{""});
@@ -145,15 +133,16 @@ TEST_CASE("Equality operator")
     CHECK_EQ(f, f2);
 }
 
-TEST_CASE_FIXTURE(FileFixture, "Change a value with set")
+TEST_CASE("Change a value with set")
 {
     constexpr auto newValue = "NewValue"sv;
 
-    auto f = File{fileName};
+    utils::TempFile tmpFile(fileName);
+    auto f = File{tmpFile.filename()};
     f.set("Section1", "Entry1", "NewValue");
     CHECK_EQ(f.get<std::string_view>("Section1", "Entry1"), newValue);
 
-    const auto f2 = File{fileName};
+    const auto f2 = File{tmpFile.filename()};
     CHECK_EQ(f.get<std::string_view>("Section1", "Entry1"), newValue);
 }
 

diff --git a/tests/utils.h b/tests/utils.h
@@ -0,0 +1,65 @@
+/*
+ * cppIni - A C++20 library for reading and writing INI files
+ * Copyright (C) 2024 Nils Hofmann <[email protected]>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
+
+#pragma once
+
+#include <filesystem>
+
+namespace utils
+{
+
+/// \brief Class for managing lifetime of a pointer
+///
+/// \details This class is a RAII wrapper for raw pointers. It is intended to be used as automatic variable in a function
+///          or as non-static data member in a class. It takes ownership of the pointer and deletes the managed object
+///          when it goes out of scope.
+///
+/// \tparam T The type of the object managed by the pointer
+template<class T, void(* Deleter)(T*)>
+class ScopeGuard
+{
+public:
+    explicit ScopeGuard(T obj) : m_obj(obj) {}
+    ~ScopeGuard() { Deleter(&m_obj); }
+
+    T& operator*() { return m_obj; }
+
+private:
+    T m_obj;
+};
+
+/// \brief Class for managing the lifetime of a temporary copy of a file
+///
+/// \details This class is a RAII wrapper for a temporary file. It is intended to be used as automatic variable in a
+///          function or as non-static data member in a class. It takes ownership of the file and deletes the file
+///          when it goes out of scope. The file is copied during construction.
+class TempFile
+{
+public:
+    explicit TempFile(const std::string& fileName)
+    {
+        std::filesystem::copy(fileName, filename());
+    }
+    ~TempFile()
+    {
+        std::filesystem::remove(filename());
+    }
+
+    constexpr static std::string_view filename() { return "tmp.ini"; }
+};
+}