Skip to content
This repository has been archived by the owner on Sep 20, 2023. It is now read-only.

Severe memory issues in basement strings #447

Open
ndmitchell opened this issue Dec 5, 2017 · 3 comments
Open

Severe memory issues in basement strings #447

ndmitchell opened this issue Dec 5, 2017 · 3 comments
Labels
B Bug

Comments

@ndmitchell
Copy link
Contributor

By tracking down some bugs, I found quite a few places where foundation strings aren't memory safe. The example I started from was sToListStream, particularly:

    onAddr fptr (Ptr ptr) = pureST (loop start)
      where
        loop !idx
            | idx == end = z
            | otherwise  = let !(Step c idx') = PrimAddr.next ptr idx in c `k` loop idx'

The onAddr here basically ensures the fptr holding the memory is available for zero time, as long as it takes pure x to evaluate the pure part but not any of the x part. Moreover, because of laziness, there's no way a single withFinalPtr could ever work - you absolutely have to have a withFinalPtr on each PrimAddr.next. Moreover, that withFinalPtr and the corresponding touch inside it had really better occur in a fixed order with respect to accessing the underlying Addr# - so I think primIndex is fundamentally dodgy called on anything other than infinitely accessible constants. It's used heavily on String, and I think that's a source of plenty of bugs.

I suggest making primIndex in IO and fixing what it shows up, but at the very least you definitely need to fix sToListStream and sToList - and I'm certain I'll find more if you don't fix it in a way the type system prevents it.

@ndmitchell ndmitchell added the B Bug label Dec 5, 2017
@vincenthz
Copy link
Member

vincenthz commented Dec 12, 2017

it's not necessarily clear cut for every of those usage, but it's seriously dodgy for toList which can be lazy at any point; I really can't think of any other way than doing a withForeignPtr at every step of the way (apart from being slightly less lazy by doing group of 4 or 8 or ... at one time)

This need some rethink, the address based subtype is seriously underused and underspecified. I think maybe a foreign ptr equivalent of Block would be a good starting point

@ndmitchell
Copy link
Contributor Author

The fundamentally primIndex is only safe with some very complex global invariants. To my mind, that means it's broken and should never be used...

@vincenthz
Copy link
Member

vincenthz commented Dec 12, 2017

we could argue this, it's one of the few oversight of the difference between native and foreign object that cause many pain for the users. it's very likely that this problem would be solved much better with something like holdThisWhileIndex... :: FinalizerStuff# -> Addr# -> ... (instead of index :: Addr# -> ...) similar to how a ByteArray# is handled and hold natively by the RTS..

I'm not sure right now what's the best workaround here. demoting primIndex to primIndexUnsafe might be a way, and have a safe(r) primIndex which does a touch on another thing after completing operation.

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

No branches or pull requests

2 participants