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: UUID#to_slice returns a dangling pointer #7901

Merged
merged 1 commit into from Jun 24, 2019

Conversation

@ysbaddaden
Copy link
Member

commented Jun 19, 2019

I thought we had #to_unsafe_slice somewhere in the stdlib but we don't, and we probably don't want to introduce it (unsafe is bad), so I changed UUID#to_slice to allocate HEAP memory and thus return a safe slice instead, and changed the internal implementation to still take advantage of the unsafe slice. I removed UUID#to_slice and added UUID#bytes that returns a StaticArray that can be passed around, and sometimes be accessible as a slice (in safe contexts).

fixes #7894

@asterite

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

As I said in the original issue, I think we should remove to_slice and instead introduce a method to return a StaticArray. Then you return a copy which doesn't allocate, and you can invoke to_slice on it and pass it to an IO without allocating memory at all.

@ysbaddaden

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

Sure, how to name it? Just UUID#bytes?

Fix: UUID returns a dangling pointer
Removes UUID#to_slice that returned a stack pointer that could
become a dangling pointer when returned from a function (for
example).

Introduces UUID#bytes to return the binary representation of the
UUID as a StaticArray(UInt8, 16) that can be passe around without
requiring HEAP allocation.

@ysbaddaden ysbaddaden force-pushed the ysbaddaden:std-fix-uuid-dangling-pointer branch from 652ff44 to 321f2fc Jun 19, 2019

@ysbaddaden

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

I removed UUID#to_slice and added UUID#bytes instead which is a much better solution!

@bytes.to_slice
# Returns the binary representation of the UUID.
def bytes : StaticArray(UInt8, 16)
@bytes.dup

This comment has been minimized.

Copy link
@ysbaddaden

ysbaddaden Jun 19, 2019

Author Member

I know dup is probably not required, but it makes the copy explicit.

@asterite
Copy link
Member

left a comment

This is great, thank you.!

@@ -161,9 +161,9 @@ struct UUID
end
end

# Returns 16-byte slice.
def to_slice

This comment has been minimized.

Copy link
@bew

bew Jun 19, 2019

Contributor

What about adding a deprecation annotation here?
But still fix #7894 by doing @bytes.to_slice.dup until we remove the method completely

@@ -173,7 +173,7 @@ struct UUID

# Returns `true` if `other` UUID represents the same UUID, `false` otherwise.
def ==(other : UUID)
to_slice == other.to_slice
@bytes == other.@bytes

This comment has been minimized.

Copy link
@oprypin

oprypin Jun 19, 2019

Contributor

Why introduce another case of .@ for no reason?

@RX14

RX14 approved these changes Jun 24, 2019

@RX14 RX14 added this to the 0.30.0 milestone Jun 24, 2019

@RX14 RX14 merged commit 28b4dbd into crystal-lang:master Jun 24, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@RX14

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Should we create another issue for StaticArray#to_slice being unsafe?

@ysbaddaden ysbaddaden deleted the ysbaddaden:std-fix-uuid-dangling-pointer branch Jun 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.