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

Change allocation strategy for unfoldrN & add warning for fromListN #387

Merged
merged 3 commits into from
May 24, 2022

Conversation

Shimuuar
Copy link
Contributor

@Shimuuar Shimuuar commented May 1, 2021

This PR changes allocation strategy for unfoldrN{,M} to Unknown and adds warning about possible DoS for fromListN. Here is reasoning from #301

I think that changing type hint from Max to Unknown for unfoldrN/unfoldrNM is right thing to do. In addition to possible heap overflow it's poor strategy in cases when upper limit is used as some sort of safeguard and vector is usually much smaller. It just wastes memory.

On other hand such change for fromListN makes no sense. It's mostly an optimization for cases when vector size is known in advance. It allows avoid reallocation of buffers when growing vector. Yes. it allows to induce heap overflow if size if hands of attacker. Without such preallocation we could just drop fromListN or implement it as fromList . take n. I think it's better to leave function as it is and just update documentation explaining possible dangers

@lehins, what's you opinion?

Fixes #301

@Shimuuar Shimuuar requested a review from lehins May 1, 2021 18:40
@Shimuuar Shimuuar marked this pull request as draft May 23, 2021 12:21
@Shimuuar
Copy link
Contributor Author

After discussion in #301 I think that problem is deeper. Why Max and Exact hints are treated alike? (#388)

@lehins
Copy link
Contributor

lehins commented May 21, 2022

I like this change.

@Shimuuar, do you mind if we include it as-is into 0.13 and get the deeper problem with bounds tackled in the future major release? if you can rebase this on master we can merge it in. If you are busy I can do it myself next week, so let me know if you oppose changes in this PR for any reason.

@Shimuuar
Copy link
Contributor Author

Not at all. I'll rebase this PR today

Shimuuar and others added 3 commits May 23, 2022 18:48
Before functions allocated array of maximum size immediately. This is problem in
case when attacker controls size parameter and could lead to DoS. It's also
problem when size parameter is just a limit vector is usually shorter. Usual
doubling strategy looks more conservative in this function.

Fixes haskell#301
Co-authored-by: konsumlamm <[email protected]>
@Shimuuar Shimuuar marked this pull request as ready for review May 23, 2022 15:57
@Shimuuar
Copy link
Contributor Author

PR is ready. @konsumlamm thanks for proofreading

Copy link
Contributor

@lehins lehins left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you!

@lehins lehins merged commit a7b90b8 into haskell:master May 24, 2022
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Aug 19, 2022
# Changes in version 0.13.0.0

 * `mkType` from `Data.Vector.Generic` is deprecated in favor of
   `Data.Data.mkNoRepType`
 * The role signatures on several `Vector` types were too permissive, so they
   have been tightened up:
   * The role signature for `Data.Vector.Mutable.MVector` is now
     `type role MVector nominal representational` (previously, both arguments
     were `phantom`). [#224](haskell/vector#224)
   * The role signature for `Data.Vector.Primitive.Vector` is now
     `type role Vector nominal` (previously, it was `phantom`).
     The role signature for `Data.Vector.Primitive.Mutable.MVector` is now
     `type role MVector nominal nominal` (previously, both arguments were
     `phantom`). [#316](haskell/vector#316)
   * The role signature for `Data.Vector.Storable.Vector` is now
     `type role Vector nominal` (previous, it was `phantom`), and the signature
     for `Data.Vector.Storable.Mutable.MVector` is now
     `type role MVector nominal nominal` (previous, both arguments were
     `phantom`). [#235](haskell/vector#235)

     We pick `nominal` for the role of the last argument instead of
     `representational` since the internal structure of a `Storable` vector is
     determined by the `Storable` instance of the element type, and it is not
     guaranteed that the `Storable` instances between two representationally
     equal types will preserve this internal structure.  One consequence of this
     choice is that it is no longer possible to `coerce` between
     `Storable.Vector a` and `Storable.Vector b` if `a` and `b` are nominally
     distinct but representationally equal types. We now provide
     `unsafeCoerce{M}Vector` and `unsafeCast` functions to allow this (the onus
     is on the user to ensure that no `Storable` invariants are broken when
     using these functions).
 * Methods of type classes `Data.Vector.Generic.Mutable.MVector` and
   `Data.Vector.Generic.Vector` use concrete monads (`ST`, etc) istead of being
   polymorphic (`PrimMonad`, etc). [#335](haskell/vector#335).
   This makes it possible to derive `Unbox` with:
   * `GeneralizedNewtypeDeriving`
   * via `UnboxViaPrim` and `Prim` instance
   * via `As` and `IsoUnbox` instance: [#378](haskell/vector#378)
 * Add `MonadFix` instance for boxed vectors: [#312](haskell/vector#312)
 * Re-export `PrimMonad` and `RealWorld` from mutable vectors:
   [#320](haskell/vector#320)
 * Add `maximumOn` and `minimumOn`: [#356](haskell/vector#356)
 * The functions `scanl1`, `scanl1'`, `scanr1`, and `scanr1'` for immutable
   vectors are now defined when given empty vectors as arguments,
   in which case they return empty vectors. This new behavior is consistent
   with the one of the corresponding functions in `Data.List`.
   Prior to this change, applying an empty vector to any of those functions
   resulted in an error. This change was introduced in:
   [#382](haskell/vector#382)
 * Change allocation strategy for `unfoldrN`: [#387](haskell/vector#387)
 * Remove `CPP` driven error reporting in favor of `HasCallStack`:
   [#397](haskell/vector#397)
 * Remove redundant `Storable` constraints on to/from `ForeignPtr` conversions:
   [#394](haskell/vector#394)
 * Add `unsafeCast` to `Primitive` vectors: [#401](haskell/vector#401)
 * Make `(!?)` operator strict: [#402](haskell/vector#402)
 * Add `readMaybe`: [#425](haskell/vector#425)
 * Add `groupBy` and `group` for `Data.Vector.Generic` and the specialized
   version in `Data.Vector`, `Data.Vector.Unboxed`, `Data.Vector.Storable` and
   `Data.Vector.Primitive`. [#427](haskell/vector#427)
 * Add `toArraySlice` and `unsafeFromArraySlice` functions for conversion to and
   from the underlying boxed `Array`: [#434](haskell/vector#434)
@Shimuuar Shimuuar deleted the unfoldrn branch August 28, 2023 11:15
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.

unfoldrN, unfoldrNM and fromListN are dangerous
3 participants