Skip to content
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.

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

Fix Log::Metadata#dup crash with 2+ entries #13369

Merged

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Apr 20, 2023

All reference types have a default #dup implementation that allocates new storage with size equal to instance_sizeof(self). Log::Metadata uses custom allocation and instances with 2 or more entries are larger than that size, but the default #dup only copies the first entry, so anything after that is uninitialized memory and can lead to crashes: (in this case :unchecked probably comes from the symbol's internal ID, used in Mutex)

puts Log::Metadata.new(nil, {a: 1, b: 2}).dup
# a: 1, unchecked: Program hit a breakpoint and no debugger was attached

Since Log::Metadata is immutable, #dup can be a no-op. A true shallow copy would have been:

def dup : self
  data_size = instance_sizeof(self) + sizeof(Entry) * {@size - 1, 0}.max
  copy = GC.malloc(data_size).as(self)
  copy.as(Void*).copy_from(self.as(Void*), data_size)
  copy
end

@HertzDevil HertzDevil changed the title Fix Log::Metadata crash with 2+ entries Fix Log::Metadata#dup crash with 2+ entries Apr 20, 2023
@straight-shoota straight-shoota added this to the 1.9.0 milestone Apr 20, 2023
@@ -28,6 +28,14 @@ describe Log::Metadata do
m({a: 1}).extend({} of Symbol => String).should_not be_empty
end

describe "#dup" do
it "creates a shallow copy" do
Log::Metadata.empty.dup.should eq(Log::Metadata.empty)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this use:

Suggested change
Log::Metadata.empty.dup.should eq(Log::Metadata.empty)
Log::Metadata.empty.dup.should be(Log::Metadata.empty)

Copy link
Member

Choose a reason for hiding this comment

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

I think this would be testing .empty more than #dup... But yeah technically, all expectations could use be. But I think that's actually an implementation detail. It's not strictly necessary for #dup to return the same instance, but it does that for performance reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It isn't part of #dup's contract to preserve object identity of immutable reference types, one day #dup might create a shallow copy for other reasons. So eq suffices for unit tests of #dup

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants