Improve memory-safety of Semigroup instances #443

clyring · 2021-11-28T14:22:17Z

After making this comment, I was a bit disappointed to discover that the existing Semigroup instances for StrictByteString and ShortByteString were also memory-unsafe in the presence of Int overflow. Here's a quick pass adding some overflow checks where they are necessary, along with a couple of other related small improvements.

Bodigrim · 2021-11-28T16:34:36Z

Is it possible to add regression tests? At least for 32-bit platforms.

clyring · 2021-11-29T04:11:48Z

To some extent, sure. I could evaluate and catch expected-exceptions from (potentially-tricky) overflow-nonsense-values inside an IO property. That seems a bit strange, though. Is there a more natural approach for this that I'm ignorant of? Should I write some?

Bodigrim · 2021-11-29T19:51:14Z

@clyring yes, via ioProperty, e. g.,

prop = ioProperty $ do
  t <- try (evaluate foo)
  pure $ isLeft t

Bodigrim

Sorry for delay, been busy with 0.11.2.0 release.

Data/ByteString/Builder/Internal.hs

Data/ByteString/Internal.hs

tests/Properties.hs

Data/ByteString/Internal.hs

clyring · 2021-12-12T03:57:00Z

Looking at the Core, it seems the rewrite rules on checkedToInt don't yet achieve the desired result even if I add SPECIALIZE pragmas to times. And of course a few cosmetic changes are wanted. But is this direction appropriate?

Data/ByteString/Internal.hs

clyring · 2021-12-14T02:08:33Z

I've reported the build failure with 9.2.1 at GHC's issue tracker.

Bodigrim · 2021-12-18T13:26:54Z

@clyring any plans to resume the work here?

clyring · 2021-12-18T18:21:59Z

I'll try to rebase and clean up a bit more tomorrow.

Some nearby low-hanging fruit are also gathered.

The 'timesInt2#' primop just doesn't exist before 9.0.1.

This causes GHC 9.2.1 to panic, for reasons I do not understand.

sjakobi

I'm pretty confused about the motivation for all these changes.

I think it would be best if you'd open proper bug reports on the issue tracker that you can reference from commit messages and comments.

Data/ByteString/Builder/Internal.hs

Data/ByteString/Internal.hs

Data/ByteString/Short/Internal.hs

tests/Properties.hs

sjakobi · 2021-12-20T00:24:50Z

I also think that this PR is getting a bit too large to review easily. If it gets any larger, maybe split it into one PR per instance.

clyring · 2021-12-20T02:20:38Z

I'm pretty confused about the motivation for all these changes.

I think it would be best if you'd open proper bug reports on the issue tracker that you can reference from commit messages and comments.

When I opened this pull request, there was one commit making a large handful of near-trivial changes, for which a separate issue would hardly be smaller than the patch, whose motivation is adequately summarized by its commit title. But inject tests and a desire to not allocate Integers unnecessarily and suddenly it becomes a bit intimidating. Funny how things happen that way... :-)

I also think that this PR is getting a bit too large to review easily. If it gets any larger, maybe split it into one PR per instance.

I agree that this PR is getting a bit large. It's already only substantially touching one instance; if the current insubstantial tweaks to the others are too distracting I can bundle them with a later PR.

sjakobi · 2021-12-20T21:25:29Z

I do think that a bug report would be very helpful. Memory safety isn't a topic that I'm encountering very frequently in Haskell code. I hope a bug report would make it clear what precisely the problem with the current code is. This will also be useful for communicating this change to bytestring users.

To simplify the review, I think having one PR per instance would be easiest.

Data/ByteString/Internal.hs

Bodigrim · 2022-02-17T20:03:52Z

@clyring @sjakobi what's the status of this?

sjakobi · 2022-02-17T21:15:18Z

Apologies for my inactivity on this PR.

Unfortunately my understanding of the problem that this PR is intended to solve is still very fuzzy. I'd like to know:

Which functions are memory-unsafe currently? Is this limited to specific implementations of Semigroup methods?
How does the memory-unsafety manifest, i.e. what happens when a size computation overflows?
Do all instances affect both 32-bit and 64-bit platforms?

@clyring Could you please write this up on the issue tracker? I think a clearer exposition of the problem will motivate us to find a solution and help guide us to the right trade-off between memory safety, performance and code complexity.

For now I have two ideas for reducing complexity:

Limiting checks to 32-bit platforms.
Limiting checks to recent GHCs.

Once we have a clearer understanding of the problem we can discuss whether and where these measures might make sense.

clyring · 2022-02-18T00:03:01Z

The merge conflicts were trivial; I have resolved them.

@clyring Could you please write this up on the issue tracker? I think a clearer exposition of the problem will motivate us to find a solution and help guide us to the right trade-off between memory safety, performance and code complexity.

This already exists an issue on the tracker for this: #456

For now I have two ideas for reducing complexity:
* Limiting checks to 32-bit platforms.

* Limiting checks to recent GHCs.

None of the checks are different between 32- and 64-bit platforms. Some of the testing code is already skipped on 64-bit platforms; specifically, the ones that would need to allocate and traverse multi-gigabyte data on 64-bit systems.
I'm not attached to the GHC-8.x compatibility CPP in the current version of this pull request. I'd personally be more inclined to just use the fallback implementation even on 9.x than to remove the checks for 8.x. It's likely only one or two CPU cycles slower and two or thee instructions larger on most architectures, which is pretty negligible. (If not for 8.x compatibility, checkedToInt might also just look at Integer constructors instead of allocating and comparing with a second Integer object.)

I'm somewhat unhappy with the current implementation's need for ugly inline hacks and the Note [stimes negativity checks], and am for that reason tempted to rewrite checkedToInt to return explicitly Overflow or Underflow instead of an uninformative Nothing. And then the checked arithmetic stuff becomes substantial enough that I'm tempted to move it into its own module.

sjakobi

Regarding the perf issues, my suggestion is to wait and see whether the WIP improvements in GHC (https://gitlab.haskell.org/ghc/ghc/-/issues/20361) will take care of this. We can record a reminder on our issue tracker.

Data/ByteString/Internal.hs

Its ByteString argument is also marked as strict. This doesn't change the optimized code produced by GHC, but allows it to be consistent with the published imprecise exception semantics.

sjakobi

Thanks!

clyring · 2022-02-28T01:55:05Z

Amusingly, the intermediate-Integer sub-issue does not represent a regression with respect to master, because times does not seem to specialize at all unless I add an INLINABLE pragma. I'm willing to hold off on re-inserting the rewrite rules in anticipation of expected improvements to Integer-related simplification in GHC 9.4.

Bodigrim · 2022-02-28T23:16:34Z

Thanks @clyring!

sjakobi · 2022-03-01T00:26:41Z

Regarding the perf issues, my suggestion is to wait and see whether the WIP improvements in GHC (https://gitlab.haskell.org/ghc/ghc/-/issues/20361) will take care of this. We can record a reminder on our issue tracker.

I have opened #493 as a reminder.

Bodigrim added this to the 0.12.0.0 milestone Dec 4, 2021

Bodigrim reviewed Dec 11, 2021

View reviewed changes

Bodigrim reviewed Dec 12, 2021

View reviewed changes

Data/ByteString/Internal.hs Outdated Show resolved Hide resolved

clyring added 7 commits December 18, 2021 21:36

Improve memory-safety of Semigroup instances

f4cf03e

Some nearby low-hanging fruit are also gathered.

Add tests for Int overflows in concat-like functions

92945f3

Incorporate cosmetic suggestions from review

b0b33ab

Adjust conversion and overflow detection for stimes

d3d1cb6

Implement checked multiplication for older GHCs

9a17831

The 'timesInt2#' primop just doesn't exist before 9.0.1.

'Fix' poor specialization behavior of strict stimes

9906555

This causes GHC 9.2.1 to panic, for reasons I do not understand.

More stimes-related cleanup and bug fixes

554506d

clyring force-pushed the Semigroup branch from d83f296 to 554506d Compare December 19, 2021 22:43

sjakobi reviewed Dec 20, 2021

View reviewed changes

sjakobi mentioned this pull request Dec 21, 2021

Memory-unsafety in Semigroup and Monoid instances #456

Closed

clyring mentioned this pull request Dec 22, 2021

The (Monoid StrictByteString) instance may be a LoopBreaker #457

Closed

clyring added 4 commits December 29, 2021 09:07

Remove safety-unrelated changes to other instances

16f74ff

Make several essentially cosmetic adjustments

e03efe8

Subtly adjust the negative-input check for stimes

36b2572

Resolve conflict between master and topic branch

a24c7ef

Bodigrim reviewed Jan 6, 2022

View reviewed changes

Data/ByteString/Internal.hs Outdated Show resolved Hide resolved

Resolve another merge conflict

6e28457

clyring added 2 commits February 27, 2022 10:26

Add basic benchmark for strict stimes

dbfcdf4

Remove efforts to avoid Integer in stimes

e4decfe

sjakobi reviewed Feb 27, 2022

View reviewed changes

Data/ByteString/Internal.hs Outdated Show resolved Hide resolved

Data/ByteString/Internal.hs Outdated Show resolved Hide resolved

clyring added 3 commits February 27, 2022 20:31

Add comments for 'checkedIntegerToInt'

068a4c1

Move/add INLINE pragmas for checked arithmetic

2d6c6f4

Reduce arity of stimesPolymorphic

8136ed3

Its ByteString argument is also marked as strict. This doesn't change the optimized code produced by GHC, but allows it to be consistent with the published imprecise exception semantics.

sjakobi approved these changes Feb 28, 2022

View reviewed changes

Bodigrim approved these changes Feb 28, 2022

View reviewed changes

Bodigrim merged commit 1256378 into haskell:master Feb 28, 2022

Bodigrim linked an issue Feb 28, 2022 that may be closed by this pull request

Memory-unsafety in Semigroup and Monoid instances #456

Closed

clyring mentioned this pull request May 18, 2022

Revisit efficiency of Integral conversions in stimes implementation for StrictByteString #493

Open

clyring mentioned this pull request Aug 7, 2022

Remove internal uses of withForeignPtr #536

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve memory-safety of Semigroup instances #443

Improve memory-safety of Semigroup instances #443

clyring commented Nov 28, 2021

Bodigrim commented Nov 28, 2021

clyring commented Nov 29, 2021 •

edited

Loading

Bodigrim commented Nov 29, 2021

Bodigrim left a comment

clyring commented Dec 12, 2021

clyring commented Dec 14, 2021

Bodigrim commented Dec 18, 2021

clyring commented Dec 18, 2021

sjakobi left a comment

sjakobi commented Dec 20, 2021

clyring commented Dec 20, 2021

sjakobi commented Dec 20, 2021

Bodigrim commented Feb 17, 2022

sjakobi commented Feb 17, 2022

clyring commented Feb 18, 2022

sjakobi left a comment

sjakobi left a comment

clyring commented Feb 28, 2022

Bodigrim commented Feb 28, 2022

sjakobi commented Mar 1, 2022

Improve memory-safety of Semigroup instances #443

Improve memory-safety of Semigroup instances #443

Conversation

clyring commented Nov 28, 2021

Bodigrim commented Nov 28, 2021

clyring commented Nov 29, 2021 • edited Loading

Bodigrim commented Nov 29, 2021

Bodigrim left a comment

Choose a reason for hiding this comment

clyring commented Dec 12, 2021

clyring commented Dec 14, 2021

Bodigrim commented Dec 18, 2021

clyring commented Dec 18, 2021

sjakobi left a comment

Choose a reason for hiding this comment

sjakobi commented Dec 20, 2021

clyring commented Dec 20, 2021

sjakobi commented Dec 20, 2021

Bodigrim commented Feb 17, 2022

sjakobi commented Feb 17, 2022

clyring commented Feb 18, 2022

sjakobi left a comment

Choose a reason for hiding this comment

sjakobi left a comment

Choose a reason for hiding this comment

clyring commented Feb 28, 2022

Bodigrim commented Feb 28, 2022

sjakobi commented Mar 1, 2022

clyring commented Nov 29, 2021 •

edited

Loading