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

Box(T).unbox crashes if T is a nilable union type #11839

Closed
HertzDevil opened this issue Feb 17, 2022 · 6 comments · Fixed by #13893
Closed

Box(T).unbox crashes if T is a nilable union type #11839

HertzDevil opened this issue Feb 17, 2022 · 6 comments · Fixed by #13893

Comments

@HertzDevil
Copy link
Contributor

class Foo; end

x = nil.as(Foo?)
Box(Foo?).unbox(Box.box(x)) # Invalid memory access (signal 11) at address 0x8

Box.box casts its argument to Void* directly if it is reference-like, via the Reference? parameter restriction. Box(T).unbox however uses the check T <= Reference || T == Nil, which fails if T is a nilable reference type, so the boxed value is treated like a box for a value type instead. The correct check is T <= Reference?, but this is currently disallowed:

In src/box.cr:30:16

 30 | {% if T <= Reference? %}
                 ^--------
Error: can't use Reference as a generic type argument yet, use a more specific type

Nothing actually stops us from permitting Reference here, as long as we ensure that unions of direct subclasses of Reference do not merge into Reference (the way it is now); the same goes for runtime checks, i.e. x.is_a?(Reference?). Without this, the workaround would be not using the Void* cast for nilable references.

Related: #11833

@HertzDevil
Copy link
Contributor Author

Expanding the subtyping relationship manually, we could probably write T.union_types.all? { |t| t <= Reference || t <= Nil }.

@icy-arctic-fox
Copy link
Contributor

This can also occur with a union of a value and nil.

x = nil.as(Int32?)
Box(Int32?).unbox(Box.box(x))

icy-arctic-fox added a commit to icy-arctic-fox/spectator that referenced this issue Jul 19, 2022
Using Box.unbox on a nil value with a union type causes:

Invalid memory access (signal 11) at address 0x8

Related Crystal issue: crystal-lang/crystal#11839
Fixes: https://gitlab.com/arctic-fox/spectator/-/issues/76
@asterite
Copy link
Member

@icy-arctic-fox 🤔 Box is meant for reference types... do you have a use-case for a union of int and nil? Or was it an accident?

@asterite
Copy link
Member

Ah, nevermind, maybe there's a use case, I don't know... at least there's a spec for values.

@icy-arctic-fox
Copy link
Contributor

My actual use case was a union of a struct and nil. But this issue applies to reference and values types when they are unioned with nil. An example use case of using Int32? would be for a config file that can have the value omitted.

icy-arctic-fox added a commit to icy-arctic-fox/spectator that referenced this issue Mar 28, 2023
@HertzDevil
Copy link
Contributor Author

HertzDevil commented Oct 16, 2023

Int32 breaks because there is a dynamic dispatch between .box(Reference?) and .box(_), so a call like Box.box(1 if rand > 0.5) actually calls either Box(Int32).box(object) or Box(Nil).box(r : Reference?). There should be no overloading here

@HertzDevil HertzDevil changed the title Box(T).unbox crashes if T is a nilable reference type Box(T).unbox crashes if T is a nilable union type Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants