Blake3Extensions.AsSpan() Extension Method is error-prone
Liam2349 opened this issue · 5 comments
First of all, awesome project! I'm currently using it for file checksums, and it's going great.
I just want to report some unexpected behaviour.
I had a sub-method that called the AsSpan() extension method on a Hash. This sub-method returned the Span to other code.
I found that the data in this Span would change on its own. I think the expectation is that when you are returned a hash, that hash is somewhat immutable.
I have inspected the source code and noticed that the extension method returns a reference to the struct's backing memory at that point in time.
In my case, the reason this data changes is because the stack unwinds after returning from the sub-method, meaning the referenced data is no longer valid; but equally if this Hash were boxed, the garbage collector can move managed heap memory unless it is pinned.
In my case, it seems to function fine if the Span is consumed immediately after being obtained. This should be true if the Hash exists on the stack, and I believe should even survive further stack allocations as they are placed after the end of that memory region; but if it is on the heap, the GC could move it at any time.
I think there should be a warning when using this method, and in its current state, perhaps Hash should be restricted to the stack, e.g. make it a ref struct. I don't think this Span is guaranteed to be correct at any point in time if the Hash is heap-allocated, e.g. as a member of an object.
You could then have a warning along the lines of "The data in this span is short-lived. If you are not consuming it immediately, please copy it to another memory location".
I did read the documentation that I could find, and saw no mention of this. I feel it needs addressing, at minimum with a warning.
In my case, it seems to function fine if the Span is consumed immediately after being obtained.
Yes, it was the purpose of the usage but agreed that it can lead to unexpected behavior. Hash being a struct, the span on it is a ref to this struct.
I think there should be a warning when using this method,
There is no such a thing. The best we can do is to rename the method to AsSpanUnsafe() and add a comment that clarifies the lifetime of the span that should match the lifetime of the Hash.
I don't think this Span is guaranteed to be correct at any point in time if the Hash is heap-allocated, e.g. as a member of an object.
It can actually, better than if it is on the stack as the Hash address (taken from a field) will be kept alive by the Span.
I think the issue is that Span is intended to point to any arbitrary memory, and by default I think we expect that to be safe. I like your proposed AsSpanUnsafe(), and I do appreciate the efficiency of the design.
With the warning, I meant as documentation on the method, e.g. an XML comment, and also as an example on the front page of this repo.
I did some heap testing and couldn't get it to fault. I'm not sure exactly how that works, since it seems to go through to Unsafe.As, which doesn't seem to have an implementation in the .NET source code, but I'll keep it on the stack anyway.
I did some heap testing and couldn't get it to fault. I'm not sure exactly how that works, since it seems to go through to Unsafe.As, which doesn't seem to have an implementation in the .NET source code, but I'll keep it on the stack anyway.
It should not fault. A Span holding a ref to a field of a managed object will correctly keep the managed object alive. In the case of the span to a stack value, ideally we should have a way to instruct the compiler so that the Span has the same lifetime than the ref valuetype passed by parameter, but in case we can't, then I will have to convert this method to unsafe.
Fixed in upcoming 0.4.0+
This is breaking in 1.0.0: I have removed AsSpanUnsafe() to AsSpan() but using proper UnscopedRef attribute (introduced in C# 11.0) so that usage of Span now should be properly protected.