Skip to content

Conversation

@Bodigrim
Copy link
Contributor

It appears that Timsort implementation accesses memory out-of-bounds, as can be witnessed by enabling vector +unsafechecks flag: https://travis-ci.org/Bodigrim/vector-algorithms/jobs/618361005#log-container

The tricky part is that while the algorithm reads some memory garbage, the latter never makes way into the final result. That is why the bug remained unspotted for so long. In its essence mergeLo and mergeHi execute the following recursive function:

iter :: MVector s a -> Int -> a -> ST s ()
iter vec i vi
  | i < 0 = return ()
  | otherwise = do
    doSmthWith vi
    vi' <- unsafeRead vec (i - 1)
    iter vec (i - 1) vi'

As one can see, iter reads vec ! (-1), but never actually uses it, so the output remains correct. However, if you are unlucky, unsafeRead will trigger a memory access violation.


This PR is just a straightforward fix. I understand that it may lack some elegance, and I believe that in future Timsort implementation may benefit from more love. But at the moment I would appreciate a reasonably quick turnaround, because for certain inputs this bug causes a segfault in poly package, which may affect its downstream dependencies (CC @sdiehl @Multramate).

install:
- cabal-1.24 update
- cabal-1.24 install --only-dependencies --enable-tests --enable-benchmarks
- cabal-1.24 install --constraint "vector +unsafechecks" --only-dependencies --enable-tests --enable-benchmarks
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Owner

@erikd erikd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks!

@erikd erikd merged commit a447a84 into erikd:master Nov 29, 2019
@Bodigrim
Copy link
Contributor Author

Bodigrim commented Dec 1, 2019

Can we have it released on Hackage please?

@erikd
Copy link
Owner

erikd commented Dec 1, 2019

Done!

@Bodigrim
Copy link
Contributor Author

Bodigrim commented Dec 1, 2019

Huge thanks for quick responses!

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

Successfully merging this pull request may close these issues.

2 participants