std::sort causes segmentation fault in operator<

Question

I am trying to sort a list (std::vector) of 3D integer vectors (IntVec). Somehow, std::sort causes a Segmentation Fault in the operator< of IntVec. Here is my code:

#include <iostream>
#include <algorithm>
#include <vector>
#include <fstream>

struct IntVec
{
public:
    long x;
    long y;
    long z; // Using ints does not cause the Segmentation Fault ?!

    friend bool operator<(const IntVec &lhs, const IntVec &rhs)
    {
        return (lhs.z < rhs.z) ||  // Segmentation Fault happens here
                    ((lhs.z == rhs.z) && (lhs.y < rhs.y))
                        || ((lhs.y == rhs.y) && (lhs.x < rhs.x));
    }
};

int main(void)
{
    std::vector<IntVec> vec;

    const int N = 2178;
    std::ifstream s("res.txt");
    for (int i = 0; i < N; i++)
    {
        IntVec t;
        s >> t.x;
        s >> t.y;
        s >> t.z;
        vec.push_back(t);
    }

    // Using vec.begin() and vec.end() does not change anything
    std::sort(vec.data(), vec.data() + vec.size());
}

I can provide you the dataset; however, I wanted to see at first if I have some big conceptual mistake in my code or some error I do not see. I found that the problem is specific to that dataset. If I leave out one entry, the Segfault does not occur. I think that is quite weird, since such a mistake should be more obvious and related to memory management. Also notice that using integers for x,y and z does not cause any problems.

Here is a godbolt version of the code.

Here is a related SO question. However, I think that my code does not have the same flaw that caused this error. I think my ordering relation is "strictly <".

Answer 1

Your operator's logic is broken (does not satisfy strict weak ordering requirement). The final clause needs a lhs.z == rhs.z too. Otherwise, lhs.z can be > rhs.z but you still get a positive result, leading to an inconsistent ordering.

Standard library algorithms put the onus on you to get this right, and breaking the resulting assumptions can easily lead to chaos (read: undefined behaviour) such as segmentation faults.

Imagine a comment inside the implementation, saying something like "at this point, we know that a is less than b, so we do not need to perform any range/bounds check on b". When a is unexpectedly larger than b, that lack of bounds checking may cause bad memory accesses. The outcome can, however, be far more subtle and cause weird bugs down the line, so it's important to get right.

You might wish to consider using a shorter, less error-prone method of implementing this ordering:

return std::tie(lhs.z, lhs.y, lhs.x) < std::tie(rhs.z, rhs.y, rhs.x);

Using operator< on a tuple (which is what std::tie gives you) automatically (and correctly!) performs the lexicographic breakdown for you.

There's actually a nice example of this on the cppreference page for std::tie, showing that it's a common thing to do.