From 00ccde23ef894c7a024d3db5cf5886ed5e9572a9 Mon Sep 17 00:00:00 2001 From: Radoslaw Zarzynski Date: Tue, 26 Jan 2021 22:54:20 +0100 Subject: [PATCH] crimson: fix dangling ClientRequest::this pointer. `ShardServices::start_operation()` spawns an operation, calls `start()` on it and returns a future plus a smart pointer controlling its life-time. Callers are responsible to ensure the pointer doesn't go out-of-scope before the entire execution is finished. This is error-prone. `OSD::handle_osd_op()` forgets about its responsibility which results in dangling `this` pointer of `ClientRequest`. I believe the problem is much wider spread and the class is just the tip of the iceberg. In this commit `start_operation()` is altered to extend the life-time a bit. However, this isn't an ultimate solution as callers are still able to e.g. put extra `this`-catching lambdas on the returned future. A new `with_operation()`- like interface is expected as a follow-up. Signed-off-by: Radoslaw Zarzynski --- src/crimson/osd/shard_services.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/crimson/osd/shard_services.h b/src/crimson/osd/shard_services.h index 2957639c63471..10f3395906ab7 100644 --- a/src/crimson/osd/shard_services.h +++ b/src/crimson/osd/shard_services.h @@ -97,7 +97,13 @@ public: throw crimson::common::system_shutdown_exception(); } auto op = registry.create_operation(std::forward(args)...); - return std::make_pair(op, op->start()); + auto fut = op->start().then([op /* by copy */] { + // ensure the op's lifetime is appropriate. It is not enough to + // guarantee it's alive at the scheduling stages (i.e. `then()` + // calling) but also during the actual execution (i.e. when passed + // lambdas are actually run). + }); + return std::make_pair(std::move(op), std::move(fut)); } seastar::future<> stop() { -- 2.47.3