Use safe isValidUtf8 for large inputs #470

noughtmare · 2022-01-20T17:05:26Z

Fixes #451

Bodigrim · 2022-01-20T20:12:06Z

@noughtmare could you please take care of ShortByteString as well?

bytestring/Data/ByteString/Short/Internal.hs

Lines 616 to 622 in 6ff6ed4

    
           isValidUtf8 :: ShortByteString -> Bool 
        
           isValidUtf8 sbs@(SBS ba#) = accursedUnutterablePerformIO $ do 
        
             i <- cIsValidUtf8 ba# (fromIntegral (length sbs)) 
        
             return $ i /= 0 
        
           foreign import ccall unsafe "bytestring_is_valid_utf8" cIsValidUtf8 
        
             :: ByteArray# -> CSize -> IO CInt

noughtmare · 2022-01-20T20:37:07Z

Done! Edit: now really...

sjakobi

This is a good amount of duplication – would it be difficult to refactor this code so it's shared between the ByteString and the ShortByteString versions?

Data/ByteString.hs

noughtmare · 2022-01-20T21:31:12Z

The FFI imports are very subtly different in their type and they are also used in a slightly different way, so I would say that it is hard to deduplicate.

sjakobi · 2022-01-20T21:35:18Z

The FFI imports are very subtly different in their type and they are also used in a slightly different way, so I would say that it is hard to deduplicate.

Alright. Please add cross-references then that remind us ~~to update both versions in sync~~ that changes to one version should probably also be made to the other version.

noughtmare · 2022-01-20T22:04:30Z

@sjakobi I have incorporated your suggestions.

sjakobi

Thanks!

clyring · 2022-01-20T22:17:06Z

Are we sure that every supported version of GHC will always make large ByteArray#s pinned? If not, then the safe FFI call for ShortByteString is a segfault waiting to happen. I think it's very cheap insurance to also check isByteArrayPinned# in the large ShortByteString case.

sjakobi · 2022-01-20T22:31:23Z

@clyring Good point. There's some documentation on this at https://ghc.gitlab.haskell.org/ghc/doc/users_guide/exts/ffi.html#pinned-byte-arrays, but it's unclear what the guarantees are with older GHCs, or to what extent this may change in the future. Let's add the pinnedness check.

clyring · 2022-01-20T22:31:59Z

Data/ByteString/Short/Internal.hs

+  let n = length sbs
+  i <- if n < 1000000
+     then cIsValidUtf8 ba# (fromIntegral n)
+     else cIsValidUtf8Safe ba# (fromIntegral n)


Also, is ba# guaranteed to be kept alive for the duration of this call? Or do we need some touch# or with... stuff here?

I think you are right that some kind of touch is necessary here, although I don't know what would be the best way to do that.

I think checking isPinnedByteArray# is sufficient here. We do not extract a pointer to byteArrayContent#, so it's GHC's responsibility not to garbage collect ba# before time.

Ah, so the touch# only has to be used if you actually use an Addr#, not with ByteArray#. @clyring do you agree that this doesn't need any further action?

~~My quick testing does suggest that GHC currently does keep ByteArray# foreign function parameters alive for the duration of even a safe FFI call.~~ But if this behavior is documented anywhere I wasn't able to find it. (~~I should raise a ticket on GHC's issue tracker about this.~~ see also this GHC issue) I would personally prefer some touch# anyway to be safe since it should have no runtime cost and if it turns out to have been necessary in some situation the resulting bug will be near-impossible for a user to track down.

I’m not sure what exactly you suggest to touch. byteArrayContent#?

I had in mind the ByteArray# reference itself, ba#. Though I concede it's possible that this won't do what I expect.

But, now that I check, the quick test I hacked together also fails to detect the unsafety of Addr# passing without any form of touch# or keepAlive#. So my previous comment should not be seen as persuasive evidence.

I do not follow. We have a thunk depending on ba# - how could it be garbage collected before the thunk is fully evaluated?

Well, my concern is that there might not be a thunk depending on ba# at runtime, if ba# is not subsequently used in Haskell code. I don't know the exact inner workings of GHC's implementation of safe FFI calls at runtime. If it involves creating a pinned thunk that holds on to the arguments of the FFI call, then that would obviously keep alive the ByteArray# argument. But this isn't made clear by the documentation, and it's easy to imagine an implementation that doesn't provide such a guarantee.

noughtmare · 2022-01-30T10:09:49Z

I've added the pinnedness check.

noughtmare · 2022-01-30T11:26:39Z

I've disabled the safe isValidUtf8 for short bytestrings for base versions before 4.10, because those don't have isByteArrayPinned# yet.

Data/ByteString/Short/Internal.hs

AndreasPK · 2022-02-02T14:07:22Z

I haven't looked at the code but do note my comment on the ghc issue tracker: https://gitlab.haskell.org/ghc/ghc/-/issues/21025#note_405881

This might need touch# in some places in light of this.

noughtmare · 2022-02-02T21:26:11Z

I've added the touch. It is a bit awkward. Does anyone know a better way?

Bodigrim · 2022-02-17T20:11:59Z

@clyring any opinion about touch# please?
@noughtmare could you please rebase?

clyring · 2022-02-18T00:14:38Z

Data/ByteString/Short/Internal.hs

+  i <- if n < 1000000 || not (isPinned ba#)
+     then cIsValidUtf8 ba# (fromIntegral n)
+     else cIsValidUtf8Safe ba# (fromIntegral n)
+  IO (\s -> (# touch# ba# s, () #))


I don't know of a better way to do this. As direct primop uses go, this is very clean.

Consider adding a comment linking to the earlier discussion about this or to the related GHC ticket.

Bodigrim · 2022-02-24T22:21:58Z

Rebased and merged, thanks @noughtmare!

* Use safe isValidUtf8 for large inputs * Use compat function for checking pinnedness * Touch byte array after passing it to safe FFI call

noughtmare force-pushed the safe-isvalidutf8 branch from 8b910ed to e4b5545 Compare January 20, 2022 20:36

noughtmare force-pushed the safe-isvalidutf8 branch from e4b5545 to 00b2240 Compare January 20, 2022 20:37

Bodigrim approved these changes Jan 20, 2022

View reviewed changes

Bodigrim requested a review from sjakobi January 20, 2022 20:46

sjakobi reviewed Jan 20, 2022

View reviewed changes

Data/ByteString.hs Show resolved Hide resolved

noughtmare force-pushed the safe-isvalidutf8 branch from 00b2240 to cd14a11 Compare January 20, 2022 22:02

sjakobi approved these changes Jan 20, 2022

View reviewed changes

Bodigrim added this to the 0.11.3.0 milestone Jan 20, 2022

clyring reviewed Jan 20, 2022

View reviewed changes

noughtmare force-pushed the safe-isvalidutf8 branch from cd14a11 to a4ef5d9 Compare January 30, 2022 10:08

noughtmare force-pushed the safe-isvalidutf8 branch from a4ef5d9 to e3cc90a Compare January 30, 2022 11:20

clyring reviewed Jan 30, 2022

View reviewed changes

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

clyring approved these changes Feb 18, 2022

View reviewed changes

noughtmare added 3 commits February 24, 2022 20:49

Use safe isValidUtf8 for large inputs

1869a25

Use compat function for checking pinnedness

b33a64a

Touch byte array after passing it to safe FFI call

631c93d

Bodigrim force-pushed the safe-isvalidutf8 branch from 466d5d5 to 631c93d Compare February 24, 2022 21:12

Bodigrim merged commit 38889aa into haskell:master Feb 24, 2022

Bodigrim pushed a commit to Bodigrim/bytestring that referenced this pull request Feb 24, 2022

Use safe isValidUtf8 for large inputs (haskell#470)

84a7070

* Use safe isValidUtf8 for large inputs * Use compat function for checking pinnedness * Touch byte array after passing it to safe FFI call

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use safe isValidUtf8 for large inputs #470

Use safe isValidUtf8 for large inputs #470

noughtmare commented Jan 20, 2022

Bodigrim commented Jan 20, 2022

noughtmare commented Jan 20, 2022 •

edited

Loading

sjakobi left a comment

noughtmare commented Jan 20, 2022

sjakobi commented Jan 20, 2022 •

edited

Loading

noughtmare commented Jan 20, 2022

sjakobi left a comment

clyring commented Jan 20, 2022 •

edited

Loading

sjakobi commented Jan 20, 2022

clyring Jan 20, 2022

noughtmare Jan 21, 2022

Bodigrim Jan 27, 2022

noughtmare Jan 30, 2022 •

edited

Loading

clyring Jan 30, 2022 •

edited

Loading

Bodigrim Jan 30, 2022

clyring Jan 30, 2022

Bodigrim Jan 30, 2022

clyring Jan 31, 2022

noughtmare commented Jan 30, 2022

noughtmare commented Jan 30, 2022

AndreasPK commented Feb 2, 2022

noughtmare commented Feb 2, 2022

Bodigrim commented Feb 17, 2022

clyring Feb 18, 2022

Bodigrim commented Feb 24, 2022

Use safe isValidUtf8 for large inputs #470

Use safe isValidUtf8 for large inputs #470

Conversation

noughtmare commented Jan 20, 2022

Bodigrim commented Jan 20, 2022

noughtmare commented Jan 20, 2022 • edited Loading

sjakobi left a comment

Choose a reason for hiding this comment

noughtmare commented Jan 20, 2022

sjakobi commented Jan 20, 2022 • edited Loading

noughtmare commented Jan 20, 2022

sjakobi left a comment

Choose a reason for hiding this comment

clyring commented Jan 20, 2022 • edited Loading

sjakobi commented Jan 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noughtmare Jan 30, 2022 • edited Loading

Choose a reason for hiding this comment

clyring Jan 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noughtmare commented Jan 30, 2022

noughtmare commented Jan 30, 2022

AndreasPK commented Feb 2, 2022

noughtmare commented Feb 2, 2022

Bodigrim commented Feb 17, 2022

Choose a reason for hiding this comment

Bodigrim commented Feb 24, 2022

noughtmare commented Jan 20, 2022 •

edited

Loading

sjakobi commented Jan 20, 2022 •

edited

Loading

clyring commented Jan 20, 2022 •

edited

Loading

noughtmare Jan 30, 2022 •

edited

Loading

clyring Jan 30, 2022 •

edited

Loading