0

Is the following code undefined behaviour or legal?

#include <thread>
#include <functional>
#include <atomic>

std::atomic<bool> b{false};

// these are defined in some other file. 
// actual implementation is not important for this question here
Handle insideFoo, continueFoo; // some type defined somewhere
void Wait(Handle h); // blocks current thread until someone calls Continue(h)
void Continue(Handle h); // makes any thread blocked in Wait(h) continue


struct S {
  int i;
  void foo() {
    i = 23; // <- sample code that uses "this" pointer
    Continue(insideFoo); // <- signal main that we are inside S::foo and past all "this" usage
    Wait(continueFoo); // <- block until main tells us to continue
    // here, "this" is destroyed (see main function)
    if (b) return; // <- b is not instance variable, so its not an access to dangling "this"
    i = 42; // <- other sample code that uses "this" pointer
  }
};

int main() {
  S* s = new S;
  continueFoo.lock();
  std::thread t(std::bind(&S::foo, s));
  Wait(insideFoo); // wait until S::foo is finished accessing "this"
  delete s;
  b = false;
  Continue(continueFoo); // let s.foo continue.
}

The question is about the fact that S::foo already started and is guaranteed to be at an instruction inside the function that does not access the this pointer anymore. Also, this "instruction inside foo()" is protected by a memory fence (std::mutex in my example here). The code that follows if(b) is also fenced (by the fact that b is a std::atomic<bool>), right?

To me, it still feels kind of fishy to return to a member function whose this-pointer is dangling, but I can't find any reason why this should be explicitly "undefined behaviour". Its not accessing s->foo() (because the function already started) and the immediate return after the check to b guarantees that no other this-access happens.

So is this allowed? Is purely returning from a subfunction (Wait(continueFoo) in this case) into a member-function whose this is dangling already undefined behaviour? I could not find any reference in the standard to that.

PS: This is a second attempt on my (poorly explained) question at Undefined behaviour to delete "this" when other thread is running member function that does not access "this"?. I don't think its the same question as the linked Is this undefined behaviour in C++ calling a function from a dangling pointer because I am not dereferencing s after its deleted. I am explicitly first calling s->foo and then deleting the instance (and I guarantee that the thread already started).

Jarod42
  • 203,559
  • 14
  • 181
  • 302
  • 1
    "*Please ignore the strange usage of std::mutex as a signal/event system*" I can't, because none of it makes any kind of sense. Locking the same mutex twice is either illegal (with `::lock`) or pointless (with `scoped_lock`). Not only that, mutex locks don't work the way you're using them even if you ignore the multi-lock. If one thread has it locked, another thread cannot *unlock* it. So it's not clear what these mutexes are intended to accomplish. Also, where is the "dangling" pointer here? Nothing is ever destroyed in `main`. – Nicol Bolas Nov 14 '19 at 14:25
  • 2
    if something is not relevant better remove it from the code, it hurts to read that some code is supposed to be ignored. Details do matter, ignoring pieces of code never lead to any good – 463035818_is_not_an_ai Nov 14 '19 at 14:26
  • @NicolBolas Ah, I missed the "delete s;" that creates the dangling pointer. As for std::mutex, locking from a different thread will block the call. – Immanuel Scholz Nov 14 '19 at 14:28
  • @NicolBolas concerning the dangling pointer i agree with you i do not really see a pointer that is dangling maybe he ment to delete s before returning. Concerning the `lock` part it is indeed legal it even is the point of `lock`. Calling lock on locked mutex blocks thread untill mutex is unlocked. – Yastanub Nov 14 '19 at 14:28
  • the question still comes off as a rather academical question, because it is not clear why you would write code like this. What is the actual problem you are trying to solve? Why would you delete an instance while another thread is using it? – 463035818_is_not_an_ai Nov 14 '19 at 14:28
  • @ImmanuelScholz: `insidefoo` is never locked by the thread, so that thread cannot `unlock` it. You also never unlock `continueFoo` in the thread. – Nicol Bolas Nov 14 '19 at 14:29
  • (ps: nothing wrong about purely academical questions, I just wonder what your actual problem is) – 463035818_is_not_an_ai Nov 14 '19 at 14:29
  • Accessing any dangling pointer leads to UB, why do you need to ask for variations of the same problem? – Slava Nov 14 '19 at 14:29
  • @formerlyknownas_463035818 the Mutexes are super-relevant for my question and I suspect my other question got prematurely closed because I did try to omit this "detail". I did not want to introduce a library whose API I have to explain or use more verbose code like condition_variables with lambdas. As far as I know, there is no simpler way of doing events in std:: c++ than to just lock/unlock on mutexes. (And I've seen quite some people that regularly use std::mutex as a signal/event replacement). – Immanuel Scholz Nov 14 '19 at 14:31
  • @ImmanuelScholz: But your "signal/event" code is *broken*. It makes no sense and does not do what your comments say they do. – Nicol Bolas Nov 14 '19 at 14:32
  • @NicolBolas ah, ok. right. I try to edit my code and remove the mutex then to get the attention to my actual question. :) – Immanuel Scholz Nov 14 '19 at 14:33
  • fwiw you do not need a third party library, there is `std::condition_variable` – 463035818_is_not_an_ai Nov 14 '19 at 14:33
  • "Is purely returning from a subfunction (continueFoo.lock in this case) into a member-function whose this is dangling already undefined behaviour?" I believe destroying an object which method is running in another thread is UB. Whatever details you have inside that method is irrelevant. – Slava Nov 14 '19 at 14:36
  • @Slava So I would assume from my gut feeling, but I can't find any evidence of that. – Immanuel Scholz Nov 14 '19 at 14:39
  • "Accessing any dangling pointer leads to UB, why do you need to ask for variations of the same problem? – Slava" Where am I accessing a dangling pointer then? – Immanuel Scholz Nov 14 '19 at 14:40
  • @formerlyknownas_463035818 yea, I was thinking about using condition_variables but did not want to get into discussions about why the need for mutexes+condition_variables and whether I misuse condition_variables and spurious wakeups or blabla, because they are not the point of the question. It's also not academical, I have a real code here (which is way too complicated to just copy and paste) – Immanuel Scholz Nov 14 '19 at 14:45
  • After reading the question and the comments, I'm puzzled by what you are actually asking. *What is allowed* and *what makes sense* are rarely the same thing, particularly in C++. – theMayer Nov 14 '19 at 14:49
  • @theMayer *"After reading the question and the comments, I'm puzzled by what you are actually asking. "* I edited my post and highlighted the essence of my question in bold. – Immanuel Scholz Nov 14 '19 at 14:56
  • 1
    The question can be simplified to: `struct foo { void bar() { delete this; /* what can I do here? */ } };` – Richard Critten Nov 14 '19 at 14:56
  • "The question can be simplified to: struct foo { void bar() { delete this; /* what can I do here? */ } };" yikes, you are right :). Thanks! How can I mark this comment as a correct answer? :) – Immanuel Scholz Nov 14 '19 at 15:04
  • @ImmanuelScholz it's not an answer because I can't fill in the comment part. – Richard Critten Nov 14 '19 at 15:07
  • "did not want to get into discussions about why the need for mutexes+condition_variables" not clear why you expect to trigger a discussion, condition variable + mutex is a valid way to signal a thread. "I have a real code here (which is way too complicated to just copy and paste)" what is the problem in your real code remains foggy. Why do you want to use an object after it has been deleted? Or the other way around: Why do you want to delete an object while it is still used? – 463035818_is_not_an_ai Nov 14 '19 at 15:09
  • 1
    This *is* a useful question, but I think the use of threads is a red herring here. You could just write `delete this;` as Immanuel Scholz said. – user253751 Nov 14 '19 at 16:18

1 Answers1

0

Richard Critten posted in comment the correct direction:

The question can be simplified to:

struct foo { void bar() { delete this; /* what can I do here? */ } };

– Richard Critten"

The delete this is a well-known phenomena and even answered explicitly in the isocpp FAQ: https://isocpp.org/wiki/faq/freestore-mgmt#delete-this

So the correct answer is basically: "Yea, its defined. Just be careful, ok?"

Community
  • 1
  • 1
  • Not going to details but what is keeping you from using destructors or simple shared pointer? – Drejc Nov 14 '19 at 15:27
  • @Drejc Simply that in the "real code" I am working with here, I don't control all parts of the codebase. I have no control over the life time (so I can't avoid the delete) and I can't e.g. join the other thread inside the destructor. (I can't even block inside the destructor). – Immanuel Scholz Nov 14 '19 at 15:35
  • If you dont control the life time then the shared pointer is a perfect solution. – Drejc Nov 14 '19 at 15:40