Skip to content

concurrency: fix placement new over the live value_ member #444

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kutsibalci wants to merge 2 commits into
base: main
Choose a base branch
from
Open

concurrency: fix placement new over the live value_ member #444

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
961c13b
concurrency: fix placement new over the live value_ member
kutsibalci Aug 4, 2026
b5adea5
concurrency: correct the rationale comment on the placement-new fix
kutsibalci Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion score/concurrency/future/interruptible_state.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
********************************************************************************/
#include "score/concurrency/future/interruptible_state.h"

#include <memory>

bool score::concurrency::InterruptibleState<void>::SetValue()
{
if ((this->TestAndMarkValueAsSet()) == true)
Expand All @@ -32,8 +34,11 @@ bool score::concurrency::InterruptibleState<void>::SetError(score::result::Error
}

// Use the constructor instead of assignment operator to circumvent issue with types that are not assignable
// The existing object is destroyed first: reusing the storage of a live object without ending its
// lifetime is undefined behaviour as soon as score::Result is not trivially destructible.
std::destroy_at(&value_);
// NOLINTNEXTLINE(score-no-dynamic-raw-memory): Non-assignable types workaround
new (&value_) score::Result<void>{MakeUnexpected<void>(error)};
static_cast<void>(::new (&value_) score::Result<void>{MakeUnexpected<void>(error)});

MakeReady();
TriggerContinuations();
Expand Down
28 changes: 22 additions & 6 deletions score/concurrency/future/interruptible_state.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "score/expected.hpp"

#include <atomic>
#include <memory>
#include <mutex>
#include <vector>

Expand Down Expand Up @@ -79,8 +80,11 @@ class InterruptibleState final : public score::concurrency::detail::TypedBaseInt
}

// Use the constructor instead of assignment operator to circumvent issue with types that are not assignable
// The existing object is destroyed first: reusing the storage of a live object without ending its
// lifetime is undefined behaviour as soon as score::Result is not trivially destructible.

@kutsibalci kutsibalci Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a fair correction and I have taken it. Placement new does end the previous object's lifetime, by reusing its storage ([basic.life]/1.5) — what it does not do is run that object's destructor, and [basic.life]/4 makes the omission undefined only for a program that depends on the destructor's side effects, not unconditionally. The old comment got both halves wrong.

Reworded at all six sites in d53f1d3, and the pull request description corrected in the same places:

// Placement-new ends the old object's lifetime by reusing its storage, but never runs its
// destructor; destroy explicitly so a non-trivially destructible score::Result releases what it holds.
std::destroy_at(&value_);

The generated code is unchanged; only the rationale moved. The fix itself still stands for the reason the description already gave — at these six call sites value_ is live, score::result::Error happens to be trivially destructible today so nothing leaks yet, and the std::destroy_at is what keeps that from becoming a real leak if it ever stops being true.

AI disclosure: AI-assisted (Claude Code). I checked the cited standard wording before accepting the correction and take responsibility for it.

std::destroy_at(&value_);
// NOLINTNEXTLINE(score-no-dynamic-raw-memory): Non-assignable types workaround
new (&value_) score::Result<Value>{std::move(value)};
static_cast<void>(::new (&value_) score::Result<Value>{std::move(value)});

MakeReady();
TriggerContinuations();
Expand All @@ -99,8 +103,11 @@ class InterruptibleState final : public score::concurrency::detail::TypedBaseInt
}

// Use the constructor instead of assignment operator to circumvent issue with types that are not assignable
// The existing object is destroyed first: reusing the storage of a live object without ending its
// lifetime is undefined behaviour as soon as score::Result is not trivially destructible.
std::destroy_at(&value_);
// NOLINTNEXTLINE(score-no-dynamic-raw-memory): Non-assignable types workaround
new (&value_) score::Result<Value>{value};
static_cast<void>(::new (&value_) score::Result<Value>{value});

MakeReady();
TriggerContinuations();
Expand All @@ -115,8 +122,11 @@ class InterruptibleState final : public score::concurrency::detail::TypedBaseInt
}

// Use the constructor instead of assignment operator to circumvent issue with types that are not assignable
// The existing object is destroyed first: reusing the storage of a live object without ending its
// lifetime is undefined behaviour as soon as score::Result is not trivially destructible.
std::destroy_at(&value_);
// NOLINTNEXTLINE(score-no-dynamic-raw-memory): Non-assignable types workaround
new (&value_) score::Result<Value>{MakeUnexpected<Value>(error)};
static_cast<void>(::new (&value_) score::Result<Value>{MakeUnexpected<Value>(error)});

MakeReady();
TriggerContinuations();
Expand Down Expand Up @@ -203,8 +213,11 @@ class InterruptibleState<Value&> final : public score::concurrency::detail::Type
}

// Use the constructor instead of assignment operator to circumvent issue with types that are not assignable
// The existing object is destroyed first: reusing the storage of a live object without ending its
// lifetime is undefined behaviour as soon as score::Result is not trivially destructible.
std::destroy_at(&value_);
// NOLINTNEXTLINE(score-no-dynamic-raw-memory): Non-assignable types workaround
new (&value_) score::Result<std::reference_wrapper<Value>>{std::ref(value)};
static_cast<void>(::new (&value_) score::Result<std::reference_wrapper<Value>>{std::ref(value)});

MakeReady();
TriggerContinuations();
Expand All @@ -219,9 +232,12 @@ class InterruptibleState<Value&> final : public score::concurrency::detail::Type
}

// Use the constructor instead of assignment operator to circumvent issue with types that are not assignable
// The existing object is destroyed first: reusing the storage of a live object without ending its
// lifetime is undefined behaviour as soon as score::Result is not trivially destructible.
std::destroy_at(&value_);
// NOLINTNEXTLINE(score-no-dynamic-raw-memory): Non-assignable types workaround
new (&value_)
score::Result<std::reference_wrapper<Value>>{MakeUnexpected<std::reference_wrapper<Value>>(error)};
static_cast<void>(::new (&value_) score::Result<std::reference_wrapper<Value>>{
MakeUnexpected<std::reference_wrapper<Value>>(error)});

MakeReady();
TriggerContinuations();
Expand Down
Loading