json
json copied to clipboard
std::tuple dangling reference - implicit conversion
What is the issue you have?
I created a nlohmann::json, which I passed to a std::apply. When I wanted to get the json value at a parameter specified function (const nlohmann::json&), it referenced some undefined memory (g++).
I tried to simplify the code, when I came across the following:
std::tuple<const nlohmann::json&>::tuple(std::tuple<nlohmann::json&>&&)
constructor creates a temporary object and a dangling reference.
This happens because nlohmann::json
implicit conversion exists from std::tuple<nlohmann::json&>
, and the tuple constructor calls the variant (3) instead of (5).
Can you provide a small but working code example?
#include <iostream>
#include <tuple>
#include <cassert>
#include "json.hpp"
int main() {
nlohmann::json j = true;
std::tuple<const nlohmann::json&> tup(std::forward_as_tuple(j));
// std::forward_as_tuple(std::as_const(j)) works well.
assert(&j == &std::get<0>(tup));
}
With clang it doesn't compile:
tuple:232:24: error: reference member '__value_' binds to a temporary object whose lifetime would be shorter than the lifetime of the constructed object
What is the expected behavior?
g++: no assertion happens on the example code. clang: compiles.
And what is the actual behavior instead?
g++: tuple creates a temporary object and creates a dangling reference. clang: not compiles.
Which compiler and operating system are you using?
- Compiler: g++ / clang (in wandbox)
- Operating system: ubuntu based (18.04) + wandbox
Which version of the library did you use?
- [ ] latest release version 3.7.3
- [ ] other release - please state the version: ___
- [X] the
develop
branch
(but all version is affected)
If you experience a compilation error: can you compile and run the unit tests?
- [X] yes
- [ ] no - please copy/paste the error message below
I have no idea how to fix this.
I have a few solution ideas.
- No implicit conversion enabled. I think this is in a future release plan (4.0.0?).
- Split "default" conversions (whose not part of basic_json) to separated includes (std::pair, std::tuple, std::unordered_map), and create a "minimal" json header which not contains these includes. The user will decide which conversion headers wants to use, and includes those manually. (I think this is the best option now)
- Make some special json conversions (ex: std::tuple) to explicit. Kind of hack smells, but can be useful at special situations. It can be design that the user will decide (with adl_serializer static constexpr bool variable?) which types are enabled only to convert explicitly.
- "default" conversions (std::tuple) disabling with macros (# ifndef) (easiest code with working single include, but ugly)
What do you think @nlohmann ?
I can create a pull request for one of from last 3 my ideas, but I need a decision, or an another acceptable solution @nlohmann .
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
bad bot!
The new version's
JSON_ImplicitConversions=OFF
and
#define JSON_USE_IMPLICIT_CONVERSIONS 0
does not solve this problem, because it isn't about FROM json conversion.
PRs welcome!
but which solution?
(Sorry, I'll have a look)
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
@schaumb, perhaps your initial example leaves original intent not entirely clear. An usable tuple<const json&>
will be created without calling forward_as_tuple
:
nlohmann::json j;
std::tuple<const nlohmann::json&> tup(j);
Tuples containing const references are somewhat dangerous, as it's easy to make these references dangling. Just like json
can be initialized from a tuple
, a string
can be initialized from character array:
std::tuple<const std::string&> tup("text");
const std::string& dangling = std::get<0>(tup); // the temporary is already destroyed
On the other hand, I tried to call std::apply
as follows:
void func(const nlohmann::json& j)
{
std::cout << j << std::endl;
}
int main()
{
nlohmann::json j{42, "hello", true};
std::apply(func, std::forward_as_tuple(j));
std::apply(func, std::forward_as_tuple(nlohmann::json{45, "hello again", false}));
}
It works as expected (see here). What is your failing use case?
My example was a way dummier than the original issue in our production code, because I simplified to the cause of the bug.
So We have a CRTP container class, where the emplace
function calls a checkEmplace
function, which can be override.
template<typename ...Types>
auto emplace(Types&&...types) {
// this is a workaround. https://github.com/nlohmann/json/issues/2226
using Tuple = std::tuple<Types&&...>;
if constexpr (sizeof...(Types) == 1 && (std::is_same_v<std::decay_t<Types>, nlohmann::json> || ...)
&& !has_checkEmplace_with_parameters_v<Derived, Tuple&&>
&& !has_checkEmplace_with_parameters_v<Derived, Collection::StdExt::template_types_helper<Tuple>, Tuple&&>) {
static_assert(Collection::StdExt::is_detected_v<Collection::StdExt::t_,
get_checkEmplace_args_impl<Derived>>, "Can not recognize checkEmplace function argument list, "
"but not match with the emplacable rvalue tuple");
using first_param = std::tuple_element_t<0, get_checkEmplace_args_t<Derived>>;
static_assert(std::is_rvalue_reference_v<first_param>, "Please make checkEmplace parameter tuple to rvalue ref");
using nlohmann_json = std::tuple_element_t<0, std::remove_reference_t<first_param>>;
if constexpr (!std::is_same_v<std::decay_t<nlohmann_json>, nlohmann::json>
|| std::is_same_v<nlohmann_json, nlohmann::json&&>) {
// nothing to do, this is not a nlohmann::json, or rvalue ref, so no problem.
} if constexpr (std::is_same_v<nlohmann_json, const nlohmann::json&>) {
return emplace(std::as_const(types)...);
} else {
static_assert(!std::is_same_v<nlohmann_json, nlohmann::json>, "Please rewrite checkEmplace parameter to const lvalue or rvalue reference.");
static_assert(!std::is_lvalue_reference_v<nlohmann_json>, "Please just don't do that yet.");
}
}
.... // locking and other stuffs
auto&& [ptr, success] = std::apply([&](auto&& ... vals) -> decltype(auto) {
return Helper::emplace(derived->container, std::forward<decltype(vals)>(vals)...);
}, derived->checkEmplace(std::forward_as_tuple(std::forward<Types>(types)...)));
... // handling result value
}
template<typename Tuple>
inline static constexpr decltype(auto) checkEmplace(Tuple&& tup) { return std::forward<Tuple>(tup); }
So I want to use as:
nlohmann::json j = nlohmann::json::parse(R"([{"test": 99}])");
container.emplace(j);
where container-s checkEmplace
function is
static std::tuple<const nlohmann::json&> checkEmplace(std::tuple<const nlohmann::json&>&& tup) { return std::tuple<const nlohmann::json&>{std::get<0>(tup).at(0)}; }
When it reaches
Helper::emplace(derived->container, ...)
, the reference is dangling.
I want to pass the j
json 0th child element const reference, without any copy.
Your example argument ("text", std::string) was not a valid reference (rvalue), so it is expected to contains a temporary object. j
is a valid reference (lvalue), so dangling reference is not expected.
https://godbolt.org/z/581Y1K
This is not a std::tuple<>
issue, because every other class, where we call
T value;
std::tuple<const T&> t = std::forward_as_tuple(value);
works well, without create any dangling reference.
nlohmann::json
the only (used) type which has implicit conversion from std::tuple<T&>
to T
, where this code is not works.
The temporary json
object is created and immediately destroyed when the argument of checkEmplace
is initialized. The desired behavior can possibly be achieved if the reference it contains is not forced to be const:
template <typename J, std::enable_if_t<std::is_same_v<const nlohmann::json&, const J&>>* = {}>
static std::tuple<const nlohmann::json&> checkEmplace(std::tuple<J&>&& tup);
Here is a slightly modified example: https://godbolt.org/z/zbn6o5
I know what is happening, but your workaround is not an acceptable solution for us.
- We are use gcc7.3 one of our target, where this code does not compile.
- The derived class
checkEmplace
cannot be template function, and cannot be overriden (because we are use some function_traits on that, where&Derived::checkEmplace
expression need to be unambiguous)
The issue is not about "How to fix our legacy code to work with this library defect", but "how to fix the library". I have listed some solution before.
@schaumb If the issue is with implicit conversions, does it go away in the latest release if you do this?
#define JSON_USE_IMPLICIT_CONVERSIONS 0
bad bot! The new version's
JSON_ImplicitConversions=OFF
and#define JSON_USE_IMPLICIT_CONVERSIONS 0
does not solve this problem, because it isn't about FROM json conversion.
@gregmarr No, as I wrote in my comment above.
Ah, yes, I remember reading that now. Is there another implicit conversion that wasn't covered by that feature?
I don't know what was the implicit conversion feature goal.
https://github.com/nlohmann/json/blob/v3.9.1/single_include/nlohmann/json.hpp#L17927 This constructor is implicit.
But if it is explicit, many features will stop working (all complicated json construction).
The issue is not about "How to fix our legacy code to work with this library defect", but "how to fix the library".
I'm not sure if this is a library defect. The assumption about perfect initialization of tuple<const T&>
from tuple<T&>
looks quite natural, but it is not a standard requirement. In general, there is probably no reason to disallow a type T
to be implicitly constructible from tuple<T&>
. This "exotic" (but legal) possibility needs to be taken into account.
There is another workaround option that will compile with gcc 7 and leaves checkEmplace
an ordinary (non-temlpate) member. The invocation of forward_as_tuple
can be moved to fallback checkEmplace
, as shown here: https://godbolt.org/z/9voq6h
To preserve compatibility with legacy code that relies on existing signature of checkEmplace
, this approach can be implemented using somewhat like checkEmplaceEx
: https://godbolt.org/z/szveeG
This "exotic" implicit conversion is not a good c++ practice.
I am sure that this is a library defect, every other class (and other used json library) will pass a simple assumption that the developers (at least ours) thought:
T obj;
std::tuple<const T&> tup(std::forward_as_tuple(obj));
assert(&obj == &std::get<0>(tup));
We have a workaround for this issue, which I copied above, so we are not "search" any other.
(This checkEmplaceEx
is not acceptable either because not only just 1 class uses checkEmplace(std::tuple<const nlohmann::json&>&& tup)
but through 3 library in 20-30 classes, and it has a refactor overhead (create new versions, etc).)
I want any of these as a solution:
- Compile flag / macro (like JSON_USE_IMPLICIT_CONVERSIONS)
- able to set a constexpr bool variable (etc
nlohmann::adl_serializer<std::tuple<nlohmann::json&>>::ImplicitFromConversion = false;
) - include magic, where we are not forced to include default
std::tuple
serialization - any other library feature we find out here
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
.
I think @karzhenkov already pointed out that lifetime prolongation for temporaries along std::forward_as_tuple is not guaranteed.
Aint this the same problem category like using std::move to a local var in decltype(auto) function?
Therefore the minimal reproduction example is already UB.
@schaumb Do you agree?
@igoloe So you would say that the issue is not related to the library?
@nlohmann as far as i understand it, it is not a problem of the library. I think its possible to build an artifical reproduction scenario with custom types. For strings at least N bytes need to be used to bypass small string optimization and end up with dangling memory.
I may check that, but can start least in 2 weeks.
Summarize the problem:
karzhenkov :
The assumption about perfect initialization of tuple<const T&> from tuple<T&> looks quite natural, but it is not a standard requirement. In general, there is probably no reason to disallow a type T to be implicitly constructible from tuple<T&>. This "exotic" (but legal) possibility needs to be taken into account.
schaumb :
This "exotic" implicit conversion is not a good c++ practice.
I think karzhenkov already pointed out that lifetime prolongation for temporaries along std::forward_as_tuple is not guaranteed.
I am not want to create any temporary object.
Aint this the same problem category like using std::move to a local var in decltype(auto) function?
No.
Therefore the minimal reproduction example is already UB.
This UB is only with nlohmann::json
class, on every other (commonly used*) class it is well defined. That is why I'm here.
*I did not find any other commonly used c++ class that works like this (no std, no boost, no poco, no qt ...)
@schaumb Do you agree?
I am not agree with that.
std::forward_as_tuple
is not creates any new or dangling nlohmann::json
object. It uses the only and one valid reference.
When I have a valid reference, I want to work like any other reference, It can be cast to const (valid) reference
without any side effect.
Again the dummy example:
nlohmann::json json;
// as from any C++ beginner book
nlohmann::json& ref = json;
const nlohmann::json& cref = ref;
std::tuple<nlohmann::json& > json_with_ref = std::forward_as_tuple(json); // works well, it stores the valid json reference
std::tuple<const nlohmann::json&> json_with_cref = json_with_ref; // oops...
I still think this is a library related issue.
It can easily be confirmed for clang 12.0 (debug, c++17) with nlohmann::json ver trunk on godbolt, if applying const&
to the source object
nlohmann::json j{};
auto const& cref{j};
std::tuple<nlohmann::json const&> const tuple1 = cref; // compile error, tries to move ...
If you modify the style you write code and use a braced initialization, you do not fall into that trap and avoid conversions at all
nlohmann::json j{};
auto const& cref{j};
std::tuple<nlohmann::json const&> const tuple1{cref}; // no move, as expected
The problem arises from the resolution of the expression
std::tuple<nlohmann::json const&> const tuple1 = cref; // compile error, tries to move ...
where it looks like assigning cref
into a tuple
, some kind of conversion is requested.
This implies implicit ctors in tuple
and matching a nlohmann::json&
to one of them.
Proof:
nlohmann::json j{};
auto& ref{j};
using Tuple = std::tuple<nlohmann::json const&>;
Tuple const tuple1 = ref; // you are asking for the conversion ctor of tuple
static_assert(std::is_constructible_v<Tuple, nlohmann::json&>); // proof, conversion ctor taken
static_assert(!std::is_copy_constructible_v<Tuple, nlohmann::json&>);); // proof, direct initialization is not taken
So we end up with the conversion ctor being selected and a dangling reference, because
- Notation with
=
favors that -
std::initializer_list
ctor innlohmann::json
is greedy and a temporarily constructed object does not participate in reference lifetime extension (inner scope).
@schaumb may work around, by using direct rather than conversion ctor.
The library may fix that, by making std::initializer_list
ctor less greedy and ambiguous. The same reason pops up, if you ask yourself, do I get an json-object or an json-array. Its always a reason to look what is being deserialized and thats a real impediment.
A modification of the current behavior is a breaking change and can only be part of a major version.
Related issues: #2583, #2311 and others i do not find.
Story continues: Using msvc 16.7.18 (vs2019) with nlohmann::json 3.9.1 on local machine behaves differently:
nlohmann::json j{};
auto const& cref{j};
std::tuple<nlohmann::json const&> const tuple1 = cref; // unexpected, not a compile error with msvc.
I remember reading a thread, that clang, msvc and gcc may behave differently regarding ctor resolution. Thats quite odd.
The exact problem was explained above. This is not the bug, the direct construction not helps.
nlohmann::json json;
// as from any C++ beginner book
nlohmann::json& ref = json;
const nlohmann::json& cref = ref;
std::tuple<nlohmann::json& > json_with_ref {std::forward_as_tuple(json)}; // works well, it stores the valid json reference
std::tuple<const nlohmann::json&> json_with_cref {json_with_ref}; // oops...
gcc did not generate error or warning.
So the problem is json_with_ref
is dangling after construction ?
No. json_with_ref
works well, the tuple contains the expected reference of json
object.
json_with_cref
is contains an unexpected json reference to a temporary object, which I didn't want to create anywhere.
The reason behind is on the issue's first comment:
std::tuple<const nlohmann::json&>::tuple(std::tuple<nlohmann::json&>&&)
constructor creates a temporary object and a dangling reference. This happens becausenlohmann::json
implicit conversion exists fromstd::tuple<nlohmann::json&>
, and the tuple constructor calls the variant (3) instead of (5).
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.