Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent false positive memory leaks from clang analyzer #553

Open
cschreib-ibex opened this issue Nov 11, 2024 · 1 comment
Open

Prevent false positive memory leaks from clang analyzer #553

cschreib-ibex opened this issue Nov 11, 2024 · 1 comment

Comments

@cschreib-ibex
Copy link
Contributor

Describe the proposed feature
We use jsoncons in one of our project, for which we run clang static analysis on every build. Unfortunately, jsoncons has had a history of triggering false positives from clang's compile-time memory leak analyzer, and this has gotten worse in 0.178.0 (we just upgraded from 0.176.0).

What other libraries (C++ or other) have this feature?
nlohmann::json

Include a code fragment with sample data that illustrates the use of this feature
Here's an example that triggers the false positive:

#include "jsoncons/json.hpp"

void test(jsoncons::json some_json, const std::string &some_string) {
  some_json["test"] = some_string;
}

Here's the compile_commands.json file (adjust with your own paths):

[
{
  "directory": ".",
  "command": "/usr/bin/clang++-16 -I jsoncons/include -o test.cpp.o -c /home/cschreib/test.cpp",
  "file": "/home/cschreib/test.cpp",
  "output": "test.cpp.o"
}
]

Running clang-tidy-16 -p . test.cpp, we get the report shown in full in "Details" below. In summary, the analyzer is unable to keep track of the storage kind of a JSON variable across function calls; it clearly sees an JSON variable being initialised as an "object" (and sees the memory allocated for that), and then immediately after it goes down an unreachable switch case where the variable is a "string" (which thus fails to de-allocate the memory allocated for the object). I don't know if something can be done to help analysers track this better.

This example reports no leak with nlohmann::json.

jsoncons/include/jsoncons/basic_json.hpp:1603:13: warning: Potential leak of memory pointed to by field 'ptr_' [clang-analyzer-cplusplus.NewDeleteLeaks]
            ::new (&cast<StorageType>()) StorageType(std::forward<Args>(args)...);
            ^
/home/cschreib/test.cpp:4:3: note: Calling 'basic_json::operator[]'
  some_json["test"] = some_string;
  ^~~~~~~~~~~~~~~~~
jsoncons/include/jsoncons/basic_json.hpp:3141:13: note: Control jumps to 'case empty_object:'  at line 3143
            switch (storage_kind())
            ^
jsoncons/include/jsoncons/basic_json.hpp:3144:17: note: Calling 'basic_json::create_object_implicitly'
                create_object_implicitly();
                ^~~~~~~~~~~~~~~~~~~~~~~~~~
jsoncons/include/jsoncons/basic_json.hpp:3713:13: note: Calling 'basic_json::create_object_implicitly'
            create_object_implicitly(typename std::allocator_traits<U>::is_always_equal());
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
jsoncons/include/jsoncons/basic_json.hpp:3723:21: note: Calling constructor for 'basic_json<char, jsoncons::sorted_policy, std::allocator<char>>'
            *this = basic_json(json_object_arg, tag());
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
jsoncons/include/jsoncons/basic_json.hpp:2793:24: note: Calling 'basic_json::create_object'
            auto ptr = create_object(alloc);
                       ^~~~~~~~~~~~~~~~~~~~
jsoncons/include/jsoncons/basic_json.hpp:1586:24: note: Calling 'allocator_traits::allocate'
            auto ptr = std::allocator_traits<stor_allocator_type>::allocate(stor_alloc, 1);
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/alloc_traits.h:464:16: note: Calling 'new_allocator::allocate'
      { return __a.allocate(__n); }
               ^~~~~~~~~~~~~~~~~
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/ext/new_allocator.h:111:2: note: Taking false branch
        if (__builtin_expect(__n > this->_M_max_size(), false))
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/ext/new_allocator.h:121:2: note: Taking false branch
        if (alignof(_Tp) > __STDCPP_DEFAULT_NEW_ALIGNMENT__)
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/ext/new_allocator.h:127:27: note: Memory is allocated
        return static_cast<_Tp*>(::operator new(__n * sizeof(_Tp)));
                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/alloc_traits.h:464:16: note: Returned allocated memory
      { return __a.allocate(__n); }
               ^~~~~~~~~~~~~~~~~
jsoncons/include/jsoncons/basic_json.hpp:1586:24: note: Returned allocated memory
            auto ptr = std::allocator_traits<stor_allocator_type>::allocate(stor_alloc, 1);
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
jsoncons/include/jsoncons/basic_json.hpp:2793:24: note: Returned allocated memory
            auto ptr = create_object(alloc);
                       ^~~~~~~~~~~~~~~~~~~~
jsoncons/include/jsoncons/basic_json.hpp:3723:21: note: Returning from constructor for 'basic_json<char, jsoncons::sorted_policy, std::allocator<char>>'
            *this = basic_json(json_object_arg, tag());
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
jsoncons/include/jsoncons/basic_json.hpp:3723:13: note: Calling move assignment operator for 'basic_json<char, jsoncons::sorted_policy, std::allocator<char>>'
            *this = basic_json(json_object_arg, tag());
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
jsoncons/include/jsoncons/basic_json.hpp:2032:13: note: Taking true branch
            if (this != &other)
            ^
jsoncons/include/jsoncons/basic_json.hpp:2034:17: note: Calling 'basic_json::move_assignment'
                move_assignment(std::move(other));
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
jsoncons/include/jsoncons/basic_json.hpp:1984:17: note: Left side of '&&' is true
            if (is_trivial_storage(storage_kind()) && is_trivial_storage(other.storage_kind()))
                ^
jsoncons/include/jsoncons/basic_json.hpp:1984:13: note: Taking false branch
            if (is_trivial_storage(storage_kind()) && is_trivial_storage(other.storage_kind()))
            ^
jsoncons/include/jsoncons/basic_json.hpp:1990:17: note: Calling 'basic_json::swap'
                swap(other);
                ^~~~~~~~~~~
jsoncons/include/jsoncons/basic_json.hpp:2408:13: note: Taking false branch
            if (this == &other)
            ^
jsoncons/include/jsoncons/basic_json.hpp:2412:17: note: Left side of '&&' is true
            if (is_trivial_storage(storage_kind()) && is_trivial_storage(other.storage_kind()))
                ^
jsoncons/include/jsoncons/basic_json.hpp:2412:13: note: Taking false branch
            if (is_trivial_storage(storage_kind()) && is_trivial_storage(other.storage_kind()))
            ^
jsoncons/include/jsoncons/basic_json.hpp:2421:17: note: Control jumps to 'case empty_object:'  at line 2424
                switch (storage_kind())
                ^
jsoncons/include/jsoncons/basic_json.hpp:2424:60: note: Calling 'basic_json::swap_l'
                    case json_storage_kind::empty_object : swap_l<empty_object_storage>(other); break;
                                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
jsoncons/include/jsoncons/basic_json.hpp:1768:13: note: Control jumps to 'case long_str:'  at line 1778
            switch (other.storage_kind())
            ^
jsoncons/include/jsoncons/basic_json.hpp:1778:53: note: Calling 'basic_json::swap_l_r'
                case json_storage_kind::long_str  : swap_l_r<TypeL, long_string_storage>(other); break;
                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
jsoncons/include/jsoncons/basic_json.hpp:1754:13: note: Calling 'basic_json::swap_l_r'
            swap_l_r(identity<TypeL>(), identity<TypeR>(), other);
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
jsoncons/include/jsoncons/basic_json.hpp:1761:13: note: Calling 'basic_json::construct'
            other.construct<TypeL>(cast<TypeL>());
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
jsoncons/include/jsoncons/basic_json.hpp:1603:13: note: Potential leak of memory pointed to by field 'ptr_'
            ::new (&cast<StorageType>()) StorageType(std::forward<Args>(args)...);
            ^
@cschreib-ibex
Copy link
Contributor Author

For people coming here with the same problem, this cannot be solved with // NOLINT commands, since this is not coming from clang-tidy but from the clang builtin analyzer. The only way that I know to silence these false positives is to add a pre-processor compilation guard, such as:

#include "jsoncons/json.hpp"

void test(jsoncons::json some_json, const std::string &some_string) {
#if !defined(__clang_analyzer__)
  // The normal code.
  some_json["test"] = some_string;
#else
  // The code that the analyzer will see.
  // Need so silence unused variable warnings...
  static_cast<void>(some_json);
  static_cast<void>(some_string);
#endif
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant