Segment fault test case.

[ghost]

I'm trying to investigate a segment fault triggered by a test case.

I don't understand what's going on yet, just use the CI system to verify my finding.

[ghost]

To avoid false alarms, can anyone confirm if this is a valid test case?

    auto m = (
        "d"
           "1:#" "i0e"
           "1:&" "d"
              "0:" "le"
              "e"
           "1:<" "le"
           "1:=" "de"
        "e"_bytes);

    std::cerr << "Start to load." << std::endl;
    MutableConfigMessage mut{m};
    std::cerr << "Start to diff." << std::endl;
    mut.diff();
    std::cerr << "Should not crash above." << std::endl;

Result:

Start to load.
Start to diff.
Randomness seeded to: 394421253

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
testAll is a Catch2 v3.1.1 host application.
Run with -? for options

-------------------------------------------------------------------------------
config message deserialization 2
-------------------------------------------------------------------------------
/drone/src/tests/test_configdata.cpp:668
...............................................................................

/drone/src/tests/test_configdata.cpp:668: FAILED:
due to a fatal error condition:
  SIGSEGV - Segmentation violation signal

[ghost]

https://github.com/oxen-io/libsession-util/blob/dev/src/config.cpp#L77

With the above test case,
config.cpp#L77 diff_impl(const set& old, const set& new_) returns a std::nullopt while
config.cpp#L105 df[key] = *diff_impl(*s, {}); tries to dereference it.

https://github.com/oxen-io/libsession-util/blob/dev/src/config.cpp#L105

[ghost]

This hack solves the crash for me and pass all the tests, but I'm not sure if it's the right fix.

diff --git a/src/config.cpp b/src/config.cpp
index 59f5c09..2fd80d0 100644
--- a/src/config.cpp
+++ b/src/config.cpp
@@ -101,9 +101,10 @@ namespace {
                 const auto& key = oldit->first;
                 if (auto* d = std::get_if<dict>(&oldit->second))
                     df[key] = *diff_impl(*d, {});
-                else if (auto* s = std::get_if<set>(&oldit->second))
-                    df[key] = *diff_impl(*s, {});
-                else
+                else if (auto* s = std::get_if<set>(&oldit->second)) {
+                    auto res = diff_impl(*s, {});
+                    res ? df[key] = *res : df.erase(key);
+                } else
                     df[key] = "-"sv;

                 ++oldit;

[jagerman]

To avoid false alarms, can anyone confirm if this is a valid test case?

It isn't valid data, but it definitely is a bug that we aren't detecting that (which is then causing this segfault because the code is assuming the input is valid). The invalidity here is caused by the empty set: those are supposed to be pruned when serializing.

However, adding that detection failed some other tests so I suspect there's a bug in the code that should be pruning as well that I'm looking into. False alarm, my initial attempt at detection wasn't allowing the top-level & to be empty (which it can be: just nothing empty inside it).

[jagerman]

Thanks for the PR and investigating this! I've pushed a fix into this PR that fixes your reported test case (which should be an exception raised for being an invalid config, definitely not a segfault).

Your proposed fix did address it, but isn't really what we want because if some external client is producing invalid data, we want to reject it so that we always produce identical serialization for the same input (in this case: we consider a missing set and an empty set to be exactly equivalent data). The same issue was there for an empty dict, which this now also tests and catches.

[ghost]

Thanks for taking care of this and implementing a proper fix!

[ghost]

if some external client is producing invalid data, we want to reject it so that we always produce identical serialization for the same input (in this case: we consider a missing set and an empty set to be exactly equivalent data).

Thanks for the explanation, that inspires me to think about some more useful patterns for property-based testing. I'll update my findings if I found new interesting things.

[jagerman]

Thanks for the explanation, that inspires me to think about some more useful patterns for property-based testing. I'll update my findings if I found new interesting things.

To extend the rationale a little: the reason we need identical output is so that de-duplication happens at the swarm level, so that if two different clients upload the same content (for example, the result of both trying to resolve a previous conflict) they end up uploading the exact same thing, which the storage server will de-duplicate and only store once.