std::tuple dangling reference - implicit conversion

[schaumb]

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));
}

try g++

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: ___
• the develop branch

(but all version is affected)

If you experience a compilation error: can you compile and run the unit tests?

• yes
• no - please copy/paste the error message below

[nlohmann]

I have no idea how to fix this.

[schaumb]

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 ?

[schaumb]

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 .

[stale]

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]

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.

[nlohmann]

PRs welcome!

[schaumb]

but which solution?

[nlohmann]

(Sorry, I'll have a look)

[stale]

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]

.

[stale]

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.

[karzhenkov]

@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?

[schaumb]

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.

[karzhenkov]

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

[schaumb]

I know what is happening, but your workaround is not an acceptable solution for us.

1. We are use gcc7.3 one of our target, where this code does not compile.
2. 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)

[schaumb]

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.

[gregmarr]

@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

[schaumb]

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.

[gregmarr]

Ah, yes, I remember reading that now. Is there another implicit conversion that wasn't covered by that feature?

[schaumb]

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).

[karzhenkov]

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

[schaumb]

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

[stale]

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]

.

[igoloe]

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?

[nlohmann]

@igoloe So you would say that the issue is not related to the library?

[igoloe]

@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.

[schaumb]

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.

[igoloe]

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

1. Notation with = favors that
2. std::initializer_list ctor in nlohmann::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.

[schaumb]

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.

[igoloe]

So the problem is json_with_ref is dangling after construction ?

[schaumb]

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 because nlohmann::json implicit conversion exists from std::tuple<nlohmann::json&>, and the tuple constructor calls the variant (3) instead of (5).

[stale]

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.

[J-B-Blankenship]

Is this still an issue?

It should be noted I expect the behavior as described by the author of this post. However, I use brace initialization as shown in the example from above:

nlohmann::json j{};
auto const& cref{j};
std::tuple<nlohmann::json const&> const tuple1{cref};

I suppose I could test this as well on the newest gcc compiler. 7 is quite old... There have been updates to evaluations of brace initialization at some point in the language standard.

[schaumb]

The issue is still exist: https://godbolt.org/z/qedM3ajW6 (with trunk gcc on master nlohmann::json)

note:

• boost::json handles this situation as compile error (preventing create dangling reference) https://godbolt.org/z/Pfq9dqsva
• JsonCpp handles as creating well defined reference (as I wanted) https://godbolt.org/z/Gxxhervn6

[iFreilicht]

I would like to second that turning off implicit conversions to basic_json would be a very welcome feature (which would fix this issue AFAIU), as we've been bitten by that as well in NixOS/nix#10087.

This does not require anything to do with std::tuple, simply defining too few overrides for all CompatibleTypes is enough!

To reproduce, clone this repo at cdb7da0f, and run:

$ clang++ main.cpp -o main && ./main
object
{"currency":"USD","value":42.99}
nestedObject
USD
objectRef
{"currency":"USD","value":42.99}
nestedObjectRef
���

The last line indicates that freed memory has been accessed.

You can then checkout iFreilicht/json-questions@e081b14, which adds a single override for object_t to prevent the implicit conversion, and see the difference:

$ clang++ main.cpp -o main && ./main
object
{"currency":"USD","value":42.99}
nestedObject
USD
objectRef
{"currency":"USD","value":42.99}
nestedObjectRef
USD

As mentioned above, JSON_USE_IMPLICIT_CONVERSIONS does not fix this issue.

@schaumb as I understand, it would be enough to skip defining constructor (3) of basic_json, would you agree?

[schaumb]

it would be enough to skip defining constructor (3) of basic_json, would you agree?

I think it is enough to make it explicit (as clang-tidy recommends it), I don't think that it should be deleted.

---

Note from me from the past:

But if it is explicit, many features will stop working (all complicated json construction).

[iFreilicht]

Yeah that seems like the more sensible option. I've started working on a PR and will see how many tests this breaks (EDIT: Not a single one it seems). I think it might well be worth the trouble and explicitly calling the constructor here and there to get additional safety, considering these implicit conversions can lead to segfaults.

Maybe it would make sense to have the switch for implicit conversions to basic_json separate from the the one for implicit conversions from basic_json, but then the name JSON_USE_IMPLICIT_CONVERSIONS wouldn't really fit anymore.

[iFreilicht]

Alright I created #4324 now to fix this.