danielaparker / jsoncons

A C++, header-only library for constructing JSON and JSON-like data formats, with JSON Pointer, JSON Patch, JSON Schema, JSONPath, JMESPath, CSV, MessagePack, CBOR, BSON, UBJSON
https://danielaparker.github.io/jsoncons
Other
726 stars 164 forks source link

Prevent false positive memory leaks from clang analyzer #553

Open cschreib-ibex opened 1 week ago

cschreib-ibex commented 1 week ago

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(std::forward(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::is_always_equal()); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ jsoncons/include/jsoncons/basic_json.hpp:3723:21: note: Calling constructor for 'basic_json>' *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::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::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>' *this = basic_json(json_object_arg, tag()); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ jsoncons/include/jsoncons/basic_json.hpp:3723:13: note: Calling move assignment operator for 'basic_json>' *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(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(other); break; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ jsoncons/include/jsoncons/basic_json.hpp:1754:13: note: Calling 'basic_json::swap_l_r' swap_l_r(identity(), identity(), other); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ jsoncons/include/jsoncons/basic_json.hpp:1761:13: note: Calling 'basic_json::construct' other.construct(cast()); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ jsoncons/include/jsoncons/basic_json.hpp:1603:13: note: Potential leak of memory pointed to by field 'ptr_' ::new (&cast()) StorageType(std::forward(args)...); ^ ```
cschreib-ibex commented 1 week ago

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
}