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

new (mutable unboxed vector) hands the user uninitialized memory #30

Closed
rwbarton opened this issue Jun 5, 2014 · 9 comments
Closed

Comments

@rwbarton
Copy link

rwbarton commented Jun 5, 2014

I don't think it should be the default behavior in a managed language to provide access to uninitialized memory. Aside from potential security issues, it also means that computations using mutable unboxed vectors in ST are nondeterministic. At the very least, this behavior should be clearly documented!

Naturally some applications need uninitialized arrays. I would suggest repurposing the name unsafeNew for this (the current unsafeNew is silly—I can't imagine any application for skipping the bounds check of an allocation), or using a new name like newUninitialized.

The same comments apply to grow and unsafeGrow.

For comparison, the array package has a newArray with an extra argument which is used as the initial value of every entry of the new mutable array, and a newArray_ which (for unboxed arrays) produces an uninitialized array when used in IO, and an array initialized to a fixed value (depending on the type) when used in ST.

@nh2
Copy link
Member

nh2 commented Jun 5, 2014

What you say seems right to me, ST shouldn't be nondeterministic. I've seen a suprising lot of code that uses new instead of one of the default value vector creation functions.

I'd also like to mention that libc's memset and friends are extremely fast as they use SSE and upwards, so initialising by default will likely be a very small penalty, assuming you will completely read or write the vector at least once afterwards.

@dolio
Copy link
Contributor

dolio commented Jul 28, 2014

I've been contemplating how best to fix this issue. I can see two or three ways, so I'd like some input.

The problem is that new is implemented in Generic. So if we want this to be resolved via an unsafe designation, we need an operation that can be called generically for this purpose.

One possibility is clear. This is currently a no-op on unboxed vectors, but we could make it zero out the array instead. Then new could call clear. This would be a bit slower, presumably, for arrays where this isn't necessary, but that may not matter.

A second option is to introduce a new operation that zeroes out unboxed arrays and no-ops on others.

A third option is to fix this even for unsafe operations, and just have basicUnsafeNew zero out unboxed arrays. This makes you unable to say, 'trust me,' for these. But that might not matter, either.

Does anyone have a strong preference for any of these solutions?

@dolio
Copy link
Contributor

dolio commented Aug 3, 2014

So, I'm having trouble actually verifying that this problem exists. Are we sure that GHC or something below it isn't zeroing the memory already?

I wrote some test cases where I allocate new primitive vectors and sum them, and I always get 0 as an answer. Have you seen something different, @rwbarton? I'm using 7.8.3, but maybe older versions worked differently (or newer ones do)? I suppose it could also be OS dependent?

@dolio
Copy link
Contributor

dolio commented Aug 3, 2014

Okay, apparently if you're using ghci, the memory will not be zeroed, but memory appears to be zeroed in compiled programs. The situation is the same for Storable vectors.

Never mind. My test case wasn't good enough. I can reproduce this in compiled programs. Storable is definitely also affected, though.

@rwbarton
Copy link
Author

rwbarton commented Aug 4, 2014

I was testing in ghci, but yeah, the OS will zero newly-allocated pages it hands you, so the unallocated area of the nursery is likely to be zeroed initially, but not after the first GC.

@treeowl
Copy link
Contributor

treeowl commented Feb 23, 2015

I have a fairly strong preference for maintaining a "trust me" option (to maintain the "performance at any cost" theme of this package). Repurposing the unsafe names for this sounds good to me personally, but uninitialized sounds fine too.

@dolio
Copy link
Contributor

dolio commented Feb 24, 2015

The problem, last I checked, is that fixing this while leaving an escape hatch requires adding operations to be very good. You need a generic initialize function that will do things for unboxed vectors and not for boxed vectors (because they are already initialized). It's rather more nice to fix by making even unsafeNew initialize the memory.

I also question whether array allocation is ever a bottleneck that requires "performance at any cost." And we need something like that to justify the more complicated solution.

@treeowl
Copy link
Contributor

treeowl commented Mar 20, 2015

I imagine it could be significant in an array-doubling situation like an unfold.

@dolio
Copy link
Contributor

dolio commented Jul 14, 2015

This has been fixed. Released in 0.11.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants