Why does std::set::insert() call operator<() with bad parameter

Question

I need help to understand why std::set<T>::insert(T) is calling operator<(const T&, const T&) with an invalid reference as its second parameter, creating a segfault upon my first insertion.

Here is the class whose objects I want to insert in a set:

/**
 * A CPC category.
 */
class Category {

  private:
    char m_section;
    int  m_class;
    char m_subclass;
    int  m_group;
    int  m_subgroup;

  public:
    Category();
    Category(char t_section, int t_class, char t_subclass, int t_group,
             int t_subgroup);
    char get_section() const;
    int  get_class() const;
    char get_subclass() const;
    int  get_group() const;
    int  get_subgroup() const;

};

Here is my operator< (sorry, not the cleanest method ever):

/**
 * Obeys simple lexical order. A smaller-than operator for Category is
 * required to allow objects to be placed in an std::set.
 */
bool operator<(const Category& cat1, const Category& cat2)
{
    return (cat1.get_section() == cat2.get_section() ?
                cat1.get_class() == cat2.get_class() ?
                    cat1.get_subclass() == cat2.get_subclass() ?
                        cat1.get_group() == cat2.get_group() ?
                            cat1.get_subgroup() == cat2.get_subgroup() ?
                                true :
                                cat1.get_subgroup() < cat2.get_subgroup() :
                            cat1.get_group() < cat2.get_group() :
                        cat1.get_subclass() < cat2.get_subclass() :
                    cat1.get_class() < cat2.get_class() :
                cat1.get_section() < cat2.get_section());
}

And here is the code creating the set:

std::istream& operator>> (std::istream& is, CategorySet& cs)
{
    std::set<Category>* cats;
    Category cat;
    while (is >> cat) {
        cats->insert(cat);
    }   
    cs = CategorySet{ cats };
    return is;
}

I put a breakpoint just before cats->insert(cat);:

Breakpoint 1, InnovationModelling::operator>> (is=..., cs=...) at lib/PatentData.cpp:126
126         cats->insert(cat);
(gdb) p *cats
$9 = std::set with 0 elements
(gdb) p cat
$1 = {m_section = 65 'A', m_class = 1, m_subclass = 65 'A', m_group = 1, m_subgroup = 0}
(gdb) ptype cat
type = class InnovationModelling::Category {
  private:
    char m_section;
    int m_class;
    char m_subclass;
    int m_group;
    int m_subgroup;

  public:
    Category(void);
    Category(char, int, char, int, int);
    char get_section(void) const;
    int get_class(void) const;
    char get_subclass(void) const;
    int get_group(void) const;
    int get_subgroup(void) const;
}

(The $9 above is because I forgot to show that the set is empty at this point so I ran the program a second time.)

Then I stepped through std::set's stuff until it calls operator(), which is not overloaded:

(gdb) s
std::less<InnovationModelling::Category>::operator() (this=0x5555555993c0 <std::cout@@GLIBCXX_3.4>, __x=..., __y=...)
    at /usr/include/c++/10.2.0/bits/stl_function.h:386
386       { return __x < __y; }
(gdb) p __x
$2 = (const InnovationModelling::Category &) @0x7fffffffd3b0: {m_section = 65 'A', m_class = 1, m_subclass = 65 'A', m_group = 1, 
  m_subgroup = 0}
(gdb) p __y
$3 = (const InnovationModelling::Category &) <error reading variable>

And sure enough, operator< gets called with this invalid reference and therefore get_section() eventually gets called on it and segfaults:

(gdb) s
InnovationModelling::Category::get_section (this=0x26) at lib/PatentData.cpp:187
187     return m_section;
(gdb) p this
$7 = (const InnovationModelling::Category * const) 0x26
(gdb) p *this
Cannot access memory at address 0x26
(gdb) s

Program received signal SIGSEGV, Segmentation fault.
0x0000555555568412 in InnovationModelling::Category::get_section (this=0x26) at lib/PatentData.cpp:187

I'm sorry if it's obvious what std::set::insert() is doing, but I'm very much a beginner in C++, and I can't even wrap my head around the fact that it feels the need to call operator<() at all to insert something in an empty set.

I'm going to guess there's something wrong with your `operator<`, most likely that it does not implement strict weak ordering correctly. It's also pretty much unreadable. Consider `return std::tie(cat1.a, cat1.b, etc) < std::tie(cat2.a, cat2.b, etc)` instead. — Retired Ninja, Oct 31 '20 at 02:16
The `operator>>()` creates an uninitialised pointer (`cats`) and repeatedly dereferences it. That gives undefined behaviour. It needs to be initialised to point at a valid object BEFORE dereferencing it. Once the behaviour of your program is undefined, all bets are off. Including the illusion you are seeing of some other function in `std::set` misbehaving. — Peter, Oct 31 '20 at 02:25
Your `operator <` returns true when the categories are equal. This is a bug. Though, probably not what causes the issue. — ALX23z, Oct 31 '20 at 02:26
@Peter It's probably not an illusion as that comparator is crazy, but you're right too — Asteroids With Wings, Oct 31 '20 at 02:26
Thank you all. I see now that my question will not be helpful to others. Should I keep it nonetheless or delete it? I have reimplemented these classes without pointers and it has fixed this issue and others I was having as well. I am not sure why I chose to implement a preorder in `operator<`, but I can see why it doesn't work with the semantics of `<` and how it could mess with `std::set`, so I fixed that too. — scozy, Oct 31 '20 at 17:37

score 2 · Accepted Answer · answered Oct 31 '20 at 02:30

And here is the code creating the set:

'Fraid not.

You created a pointer-to-set, but it doesn't actually point to any set; it's just an uninitialised pointer.

You could use new to dynamically allocate a set, but don't do that unless you have to. In fact, don't use pointers unless you have to. Your needs here will be driven by CategorySet (which we know nothing about), but if you can just make your set a nice local variable, life will be much easier.

Why does std::set::insert() call operator<() with bad parameter

1 Answers1