Skip to content

Avoid mishandling retain_n of immortal objects where n >= 3 #30034

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

Merged
merged 1 commit into from
Mar 26, 2020
Merged

Avoid mishandling retain_n of immortal objects where n >= 3 #30034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
49 changes: 45 additions & 4 deletions stdlib/public/SwiftShims/RefCount.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -805,6 +805,13 @@ class RefCounts {
// Increment the reference count.
void increment(uint32_t inc = 1) {
auto oldbits = refCounts.load(SWIFT_MEMORY_ORDER_CONSUME);

// constant propagation will remove this in swift_retain, it should only
// be present in swift_retain_n
if (inc != 1 && oldbits.isImmortal(true)) {
return;
}

RefCountBits newbits;
do {
newbits = oldbits;
Expand All @@ -820,6 +827,13 @@ class RefCounts {

void incrementNonAtomic(uint32_t inc = 1) {
auto oldbits = refCounts.load(SWIFT_MEMORY_ORDER_CONSUME);

// constant propagation will remove this in swift_retain, it should only
// be present in swift_retain_n
if (inc != 1 && oldbits.isImmortal(true)) {
return;
}

auto newbits = oldbits;
bool fast = newbits.incrementStrongExtraRefCount(inc);
if (SWIFT_UNLIKELY(!fast)) {
Expand Down Expand Up @@ -982,6 +996,12 @@ class RefCounts {
bool doDecrementSlow(RefCountBits oldbits, uint32_t dec) {
RefCountBits newbits;

// constant propagation will remove this in swift_release, it should only
// be present in swift_release_n
if (dec != 1 && oldbits.isImmortal(true)) {
return false;
}

bool deinitNow;
do {
newbits = oldbits;
Expand Down Expand Up @@ -1024,6 +1044,12 @@ class RefCounts {
bool doDecrementNonAtomicSlow(RefCountBits oldbits, uint32_t dec) {
bool deinitNow;
auto newbits = oldbits;

// constant propagation will remove this in swift_release, it should only
// be present in swift_release_n
if (dec != 1 && oldbits.isImmortal(true)) {
return false;
}

bool fast =
newbits.decrementStrongExtraRefCount(dec);
Expand Down Expand Up @@ -1066,6 +1092,12 @@ class RefCounts {
auto oldbits = refCounts.load(SWIFT_MEMORY_ORDER_CONSUME);
RefCountBits newbits;

// constant propagation will remove this in swift_release, it should only
// be present in swift_release_n
if (dec != 1 && oldbits.isImmortal(true)) {
return false;
}

do {
newbits = oldbits;
bool fast =
Expand Down Expand Up @@ -1248,7 +1280,15 @@ class RefCounts {
// Return weak reference count.
// Note that this is not equal to the number of outstanding weak pointers.
uint32_t getWeakCount() const;


// DO NOT TOUCH.
// This exists for the benefits of the Refcounting.cpp tests. Do not use it
// elsewhere.
auto getBitsValue()
-> decltype(auto) {
return refCounts.load(std::memory_order_relaxed).getBitsValue();
}

private:
HeapObject *getHeapObject();

Expand Down Expand Up @@ -1449,13 +1489,14 @@ inline bool RefCounts<InlineRefCountBits>::doDecrementNonAtomic(uint32_t dec) {
// Use slow path if we can't guarantee atomicity.
if (oldbits.hasSideTable() || oldbits.getUnownedRefCount() != 1)
return doDecrementNonAtomicSlow<performDeinit>(oldbits, dec);

if (oldbits.isImmortal(true)) {
return false;
}

auto newbits = oldbits;
bool fast = newbits.decrementStrongExtraRefCount(dec);
if (!fast) {
if (oldbits.isImmortal(false)) {
return false;
}
return doDecrementNonAtomicSlow<performDeinit>(oldbits, dec);
}

Expand Down
52 changes: 52 additions & 0 deletions unittests/runtime/Refcounting.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
using namespace swift;

struct TestObject : HeapObject {
constexpr TestObject(HeapMetadata const *newMetadata)
: HeapObject(newMetadata, InlineRefCounts::Immortal)
, Addr(NULL), Value(0) {}

size_t *Addr;
size_t Value;
};
Expand All @@ -34,6 +38,8 @@ static const FullMetadata<ClassMetadata> TestClassObjectMetadata = {
{ { nullptr }, ClassFlags::UsesSwiftRefcounting, 0, 0, 0, 0, 0, 0 }
};

static TestObject ImmortalTestObject{&TestClassObjectMetadata};

/// Create an object that, when deallocated, stores the given value to
/// the given pointer.
static TestObject *allocTestObject(size_t *addr, size_t value) {
Expand Down Expand Up @@ -220,3 +226,49 @@ TEST(RefcountingTest, nonatomic_unknown_retain_release_n) {
EXPECT_EQ(0u, value);
EXPECT_EQ(1u, swift_retainCount(object));
}

// Verify that refcounting operations on immortal objects never changes the
// refcount field.
TEST(RefcountingTest, immortal_retain_release) {
auto initialBitsValue = ImmortalTestObject.refCounts.getBitsValue();

swift_retain(&ImmortalTestObject);
EXPECT_EQ(initialBitsValue, ImmortalTestObject.refCounts.getBitsValue());
swift_release(&ImmortalTestObject);
EXPECT_EQ(initialBitsValue, ImmortalTestObject.refCounts.getBitsValue());
swift_nonatomic_retain(&ImmortalTestObject);
EXPECT_EQ(initialBitsValue, ImmortalTestObject.refCounts.getBitsValue());
swift_nonatomic_release(&ImmortalTestObject);
EXPECT_EQ(initialBitsValue, ImmortalTestObject.refCounts.getBitsValue());

swift_unownedRetain(&ImmortalTestObject);
EXPECT_EQ(initialBitsValue, ImmortalTestObject.refCounts.getBitsValue());
swift_unownedRelease(&ImmortalTestObject);
EXPECT_EQ(initialBitsValue, ImmortalTestObject.refCounts.getBitsValue());
swift_nonatomic_unownedRetain(&ImmortalTestObject);
EXPECT_EQ(initialBitsValue, ImmortalTestObject.refCounts.getBitsValue());
swift_nonatomic_unownedRelease(&ImmortalTestObject);
EXPECT_EQ(initialBitsValue, ImmortalTestObject.refCounts.getBitsValue());

for (unsigned i = 0; i < 32; i++) {
uint32_t amount = 1U << i;

swift_retain_n(&ImmortalTestObject, amount);
EXPECT_EQ(initialBitsValue, ImmortalTestObject.refCounts.getBitsValue());
swift_release_n(&ImmortalTestObject, amount);
EXPECT_EQ(initialBitsValue, ImmortalTestObject.refCounts.getBitsValue());
swift_nonatomic_retain_n(&ImmortalTestObject, amount);
EXPECT_EQ(initialBitsValue, ImmortalTestObject.refCounts.getBitsValue());
swift_nonatomic_release_n(&ImmortalTestObject, amount);
EXPECT_EQ(initialBitsValue, ImmortalTestObject.refCounts.getBitsValue());

swift_unownedRetain_n(&ImmortalTestObject, amount);
EXPECT_EQ(initialBitsValue, ImmortalTestObject.refCounts.getBitsValue());
swift_unownedRelease_n(&ImmortalTestObject, amount);
EXPECT_EQ(initialBitsValue, ImmortalTestObject.refCounts.getBitsValue());
swift_nonatomic_unownedRetain_n(&ImmortalTestObject, amount);
EXPECT_EQ(initialBitsValue, ImmortalTestObject.refCounts.getBitsValue());
swift_nonatomic_unownedRelease_n(&ImmortalTestObject, amount);
EXPECT_EQ(initialBitsValue, ImmortalTestObject.refCounts.getBitsValue());
}
}