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

DV.Generic.Mutable.fill does not check for out of bounds access #118

Open
Shimuuar opened this issue Jun 10, 2016 · 10 comments
Open

DV.Generic.Mutable.fill does not check for out of bounds access #118

Shimuuar opened this issue Jun 10, 2016 · 10 comments

Comments

@Shimuuar
Copy link
Contributor

So it should be either renamed as unsafeFill and/or bound checks added. It's possible this is not only one function with unsafe access.

module Fill where

import qualified Data.Vector.Unboxed.Mutable       as MU
import qualified Data.Vector.Generic.Mutable       as MG
import qualified Data.Vector.Fusion.Stream.Monadic as SS

main :: IO ()
main = do
  mv <- MU.replicate 10 (1 :: Int)
  MG.fill mv (SS.replicate 1000000 42)
  return ()
[1 of 1] Compiling Fill             ( /home/alexey/qqq/fill-sigsegv.hs, interpreted )
Ok, modules loaded: Fill.
*Fill> :main
Segmentation fault
@cartazio cartazio added this to the 0.12 milestone Jul 24, 2016
@dolio
Copy link
Contributor

dolio commented Jul 25, 2016

So, I looked at this.

A possible issue is that I see no way to implement a safe fill that doesn't do a lot of bounds checking. It just takes a Stream, which has no size information, so we can't just check that we're big enough and call unsafeFill. We have to check for each element.

I'll probably split it into two functions. It's listed as an "internal" function right now, so those uses will probably transition to unsafeFill. Then there will be a user-visible fill that does a bunch of arithmetic.

That's my plan, anyhow.

@Shimuuar
Copy link
Contributor Author

I think any function which could perform unchecked out of bounds access should be marked as unsafe to make clear that they can violate memory safety.

It probably make sense to expose unsafeFill as well.

@cartazio
Copy link
Contributor

considering the type of fill is
fill :: (PrimMonad m, MVector v a) => v (PrimState m) a -> Stream m a -> m (v (PrimState m) a),
and its under the header Internal operations, i dont think this can be deemed a valid bug :)

@Shimuuar
Copy link
Contributor Author

If it is internal function it should be given sufficiently scary name or moved into some *.Internal module. Also no one reads all that small print about which function is internal and which is not. At least I don't.

@cartazio
Copy link
Contributor

Read the docs. It's under internal

We can't help you with literacy

On Monday, July 25, 2016, Aleksey Khudyakov [email protected]
wrote:

If it is internal function it should be given sufficiently scary name or
moved into some *.Internal module. Also no one reads all that small print
about which function is internal and which is not. At least I don't.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#118 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAQwjjVOrKTNayo8tEIWuEUF8ua62Xtks5qZP-xgaJpZM4Iy2UW
.

@cartazio
Copy link
Contributor

Sorry. That was out of turn and inappropriate and mean . I let some
unrelated irritation this afternoon make me a little jumpy and mean without
due cause.

I hope you're having a lovely afternoon and I'm excited to collaborate.

On Monday, July 25, 2016, Carter Schonwald [email protected]
wrote:

Read the docs. It's under internal

We can't help you with literacy

On Monday, July 25, 2016, Aleksey Khudyakov <[email protected]
javascript:_e(%7B%7D,'cvml','[email protected]');> wrote:

If it is internal function it should be given sufficiently scary name or
moved into some *.Internal module. Also no one reads all that small print
about which function is internal and which is not. At least I don't.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#118 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAQwjjVOrKTNayo8tEIWuEUF8ua62Xtks5qZP-xgaJpZM4Iy2UW
.

@Shimuuar
Copy link
Contributor Author

No worries.

Still I think that exporting internal and public API from same module is wrong design. It's too easy to confuse then. Moreover same internal APIs contains stream/unstream & friends which are definitely part of public API since it's not possible to write fusible functions which operate on vector.

So in my opinion they should be either raised to status of public API or safely hidden in some Internal module.

@Shimuuar
Copy link
Contributor Author

Another point about public/internal API distinction: #70. Someone asked for API which would be marked internal so it makes sense to make it public.

@cartazio
Copy link
Contributor

I guess this does nicely line up with the discussion around exposing
internals in internal modules of each respective representation. Eg unboxed
vectors etc

On Tuesday, July 26, 2016, Aleksey Khudyakov [email protected]
wrote:

Another point about public/internal API distinction: #70
#70. Someone asked for API
which would be marked internal so it makes sense to make it public.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#118 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAQwhDfwcacvekFfovrdbMstq6j0jqIks5qZnUPgaJpZM4Iy2UW
.

@Shimuuar
Copy link
Contributor Author

Their only common point is that they about exposing/not exposing internal API. Otherwise they're largely independent. So it's probably better to discuss them separately. So we have two questions.

  1. Should we expose and document streaming API? I think since Data.Vector.Fusion.* is exposed that API should be public too.
  2. Should we export constructors for unboxed vectors and how if so? On one hand yes, since some operations couldn't be implemented efficiently without access to the. Counterpoint is their unsafety. For example (V_2 100 [] []) ! 10 will lead to out of bounds access

But this is consequence of larger problem. Representation of unboxed vectors as structure of arrays pushes borders of language expressivity. (Look at all that CPP!). Adding another instance require a lot of boilerplate and it's not possible/difficult to abstract it away.

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

No branches or pull requests

3 participants