[sigc++ 3] Use after free with signal::make_slot()

[H2NCH2COOH]

No description provided.

[H2NCH2COOH]

For the following piece of code

auto s1 = sigc::signal<void()>();
auto s2 = new sigc::signal<void()>();
s1.connect(s2->make_slot());
delete s2;
s1.emit();

valgrind complains that memory is accessed after being freed.

I tested the above code with sigc++ 2 and valgrind didn't complain.

After some digging, I found out that this seems to be caused by the commit: a3e2fe66 which made signal_base not derive from trackable.

If I revert the change, the above code won't have valgrind error.

[kjellahl]

It's even worse than just valgrind complaining. When I run your code without
valgrind, I get a segfault.

Obviously it was a mistake not to derive signal_base from trackable.
Unfortunately it's not as simple as reverting the commit that made signal_base
not trackable. That would break ABI.

The only solution I can think of right now that would not break ABI is:

• Add a warning in the documentation of signal::make_slot(). Perhaps deprecate
  signal::make_slot().
• Make a new class (trackable_signal?) that derives from trackable.

I don't like it. I hope there is a better solution.

[H2NCH2COOH]

If this can't be fixed directly, can it be avoided by adding some code to the above sample?

Like:

auto s1 = sigc::signal<void()>();
auto s2 = new sigc::signal<void()>();
auto c = s1.connect(s2->make_slot());
c.disconnect();
delete s2;
s1.emit();

Is there a better way other than disconnecting all slots created using make_slot() manually?

[kjellahl]

Manual disconnection solves the problem.

A class that derives both from sigc::signal and sigc::trackable is an alternative.

#include <sigc++/sigc++.h>
#include <iostream>

class MySignal : public sigc::signal<void()>, public sigc::trackable
{
public:
  decltype(auto) make_slot() const
  {
    return sigc::mem_fun(*this, &MySignal::emit);
  }
};

int main()
{
  auto s1 = sigc::signal<void()>();
  auto s2 = new MySignal();
  s2->connect([] { std::cout << "Hi, there\n"; });
  s1.connect(s2->make_slot());
  std::cout << "Before delete s2\n";
  s1.emit();
  delete s2;
  std::cout << "After delete s2\n";
  s1.emit();
  return 0;
}

MySignal must define its own make_slot(), because *this must denote an object
that derives from sigc::trackable. *this in sigc::signal::make_slot() does not.

[H2NCH2COOH]

I'll try the second way.
Thank you very much.

[kjellahl]

Here's the solution: Derive sigc::signal_with_accumulator from sigc::trackable.
signal_with_accumulator is a class template. It's not part of the ABI. A new
base class can be added without breaking ABI.

This fix is not automatically available just by linking to an updated library file.
An application that needs the fix must be recompiled with access to updated sigc++ header files.

[H2NCH2COOH]

Sorry if it may seem a stupid question but why is this not an ABI change?
If I have a piece of code compiled before this commit which accepts a sigc::signal<int()>, and a piece of code compiled after this commit which provides a sigc::signal<int()>, (dynamically) linking these two I believe will cause runtime problems will it not?

[kjellahl]

Since sigc::signal is a class template, the generated code is stored in the user's
library file or application. If you install an updated libsigc-3.0.so, nothing changes.
This is the simple case. I was uncertain what might happen in more complicated cases.
E.g. glibmm uses sigc::signal. Some generated code for sigc::signal is stored
in libglibmm-2.68.so. What might happen if an application uses glibmm and sigc:signal,
and a new version of libglibmm-2.68.so is installed? I made some tests (only with
the g++ compiler), and everything I tested worked.

Your question makes me uncertain again. I didn't think of two related sigc::signal
instances compiled with different versions of sigc++. Then connected with make_slot(),
I suppose? Can you test it easily?

It's definitely safer to leave sigc::signal unchanged, and add sigc::trackable_signal
and sigc::trackable_signal_with_accumulator that derives from sigc::trackable.

Today's change hasn't been released. It can still be reverted. Have you got an opinion?

[H2NCH2COOH]

I did a quick test:

liba.so built with new sigc::signal_with_accumulator inherit from sigc::trackable:

sigc::signal<void(int)> sig = {};

void f(const sigc::slot<void(int)>& s) {
    sig.connect(s);
}

void g() {
    sig(42);
}

test built with old sigc::signal_with_accumulator:

sigc::signal<void(int)>* sig = new sigc::signal<void(int)>();
sig->connect([](int i){ printf("%d\n", i); });
f(sig->make_slot());
delete sig;
g();

When test was executed, I got a segfault.

The crash won't happen if liba.so is built with old sigc and test with new.

And using old or new libsigc-3.0.so on runtime won't affect the crash.

[kjellahl]

Thanks for the test. But the result does not really tell us if there is a
serious problem with the new version of sigc::signal_with_accumulator.
I suppose the crash occurs when g() is called. That's expected when test is
built with the old sigc::signal_with_accumulator. The signal in test does not
derive from sigc::trackable. The signal in liba.so is not informed of the
delete sig in test. sig(42) in g() tries to emit the deleted signal. You would get the
same result if everything had been built with the old sigc::signal_with_accumulator.

Still I've a feeling that it's not safe to implement the new sigc::signal_with_accumulator,
deriving from sigc::trackable. It might break ABI somewhere. We've seen examples
before, where seemingly safe changes in sigc++ have caused ABI breaks in other modules.

I'm inclined to revert the fix that changes sigc::signal_with_accumulator, and add
a sigc::trackable_signal_with_accumulator that derives from sigc::trackable +
a sigc::trackable_signal that derives from sigc::trackable_signal_with_accumulator.
Most users can continue to use the old sigc::signal. Most signals don't have to be
trackable. Comments? Is sigc::trackable_signal a good name?

[H2NCH2COOH]

I did some more tests.
When test and liba are passing signals around using references, everything seems fine.
When liba has a function: sigc::signal<void(int)> f() and test calls it like auto sig = f(), it crashes when liba is built with old sigc and test with new.

[kjellahl]

It looks like you've found a real problem with my "fix". I suppose that auto sig = f()
does not crash if both liba and test are built with the old sigc. If so, it's a case
that works with the old sigc, but it crashes if only test is rebuilt with the new sigc.

[H2NCH2COOH]

I think it is indeed a problem, although not a common way of using the type.

I think sigc::trackable_signal is a good name.