Skip to content

Conversation

@hesiod
Copy link

@hesiod hesiod commented Sep 15, 2016

Add explicit Safe/Trustworthy annotations to every module.

Move the writeByteString function from Blaze.ByteString.Builder.ByteString
to Blaze.ByteString.Builder.Internal.Write because it uses rather unsafe
functions compared to the rest of the module and fits better in the
unsafe context of the latter module. This makes it possible to mark the former
module Safe. There is no impact on the public API (if you consider the Internal
modules private).

Add explicit Safe/Trustworthy annotations to every module.

Move the writeByteString function from Blaze.ByteString.Builder.ByteString
to Blaze.ByteString.Builder.Internal.Write because it uses rather unsafe
functions compared to the rest of the module and fits better in the
unsafe context of the latter module. This makes it possible to mark the former
module Safe. There is no impact on the public API (if you consider the Internal
modules private).
@lpsmith
Copy link
Owner

lpsmith commented Sep 15, 2016

Looks promising, though we do need to use CPP to support some older versions of GHC. (See the travis build...)

I'm a little unsettled by the writeByteString move, because this (presumably trustworthy) function is publically exported from a safe module. Then what does safe mean, and is safe haskell even sound?

@hesiod
Copy link
Author

hesiod commented Sep 15, 2016

The difference between Safe and Trustworthy is that the former is verified by the compiler and the latter essentially is "I assert that this module does nothing scary, even though it imports unsafe stuff". (N.B.: Whether a Trustworthy module is actually considered trustworthy depends on package trust, have a look at the SafeHaskell GHC Trac wiki page)

The reason I moved writeByteString is because it is not in line with the other functions of the module: While the other declarations in the module are really Safe, i.e. don't use anything unsafe, writeByteString uses the unsafe toForeignPtr. Now, the Internal.Write module already contains unsafe declarations and imports from unsafe modules, so it can only be Trustworthy - we can only declare that it is trustworthy by fiat.

Perhaps the rationale is best declared with the practical ramifications: If I didn't move writeByteString, Blaze.ByteString.Builder.ByteString could only be declared Trustworthy. Now, imagine someone adds some unsafe code to the module, and we would like to be warned that the code we added is in fact unsafe. But we told GHC that "you can trust all of this, it's ok!" using Trustworthy! With the writeByteString move, we can benefit from GHC statically verifying whether our code deserves to be called safe.

I hope this explains the rationale a bit better 😄

Regarding guarding the pragmas with CPP: It seems that the failure is due to GHC 7.0 not supporting Safe Haskell. However, GHC 7.0 is a bit old these days - would you consider dropping support for GHC 7.0, or is that too bold?

@lpsmith
Copy link
Owner

lpsmith commented Sep 15, 2016

Forgive my lack of understanding of safe haskell, but my point is, writeByteString seems to be "safe" even though it's only actually "trustworthy". Let's say we introduce an obvious memory fault into writeByteString; if you reject "trustworthy" code and only accept "safe" code, what's to prevent such a person from using writeByteString and introducing a memory fault in their "safe" code?

If it isn't any such mechanism, then this feels like safety-laundering, which admittedly is something more of a problem with safe haskell (i.e. it being unsound) than with bytestring-builder.

With regard to GHC 7.0, if it were another package, maybe, but since this is a compatibility package, I'm inclined to keep GHC 7.0 support going for the time being.

@hesiod
Copy link
Author

hesiod commented Sep 15, 2016

I'm definitely not an expert on Safe Haskell internals, but my understanding is that a reexport doesn't make a declaration part of another module, so writeByteString is never considered Safe. If you enforced package trust using -fpackage-trust, GHC should reject usage of writeByteString in a module declared Safe if the package is untrusted (by default, all packages are untrusted).

On a different note, I'm not yet sure whether the Internal.Write module deserves to be considered trustworthy. It's all very much a matter of interpreting the definitions of safety/trustworthiness.

I will add the required CPP magic for GHC 7.0.

@lpsmith
Copy link
Owner

lpsmith commented Sep 15, 2016

I'd have to review but I kinda doubt that Write.Internal is trustworthy, even under fairly generous definitions of trustworthiness.

And perhaps I need to dig into safe haskell a bit, but it seems to me, if you aren't going to trust blaze-builder, then you really shouldn't be able to use pretty much any of the Write module. Assuming you trust bytestring (and bytestring-builder, if applicable), then the only thing you should get to use is the pass-through functionality.

Blaze.ByteString.Builer.Internal.Write should be marked Unsafe since
it makes it easy to shoot yourself in the foot.
Add CPP logic guarding the SafeHaskel statements.
Only expose Safe or Trustworthy pragmas for GHC >= 7.2.
Only expose Unsafe pragmas for GHC >= 7.4.
Add changelog for 0.4.1.0 and bump version in the cabal file.
@hesiod
Copy link
Author

hesiod commented Sep 17, 2016

I marked Internal.Write unsafe, added the required CPP logic (let's see if GHC 7.0/7.2 on Travis likes it) and boldly added a changelog entry and bumped the version to 0.4.1.0.

Add CPP logic left out in an earlier commit.
@andreasabel
Copy link

Maintenance moved to https://github.com/blaze-builder/blaze-builder

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.

3 participants