haskoin / secp256k1-haskell

Haskell bindings for secp256k1 library
Other
24 stars 35 forks source link

[libsecp256k1] illegal argument: input != NULL #36

Closed tim2CF closed 3 years ago

tim2CF commented 3 years ago
13:32 < timCF> Hello! Is anybody experienced in Haskell<->C FFI APIs? I have a very strange issue with secp256k1 Haskell wrapper around C code. In my code I'm using this function 
               https://hackage.haskell.org/package/secp256k1-haskell-0.5.0/docs/src/Crypto.Secp256k1.html#importSig It's accepting `ByteString` and returning `Maybe Sig`. I think by design 
               it should catch all low-level errors and return Nothing in case of some 
13:32 < timCF> failure. I did randomly found that **sometimes** it's failing with C exception (I guess related to NULL pointer) in case where empty ByteString is passed to this Haskell 
               function. It's really strange that in some places empty ByteString is resolved as NULL pointer, but if I'm trying to reproduce it artificially passing ("" :: ByteString) 
               explicitly - it works just fine, returns Nothing. Are there 
13:32 < timCF> different "kinds" of empty ByteString terms in Haskell?
13:33 < timCF> Exception looks like `[libsecp256k1] illegal argument: input != NULL`
13:34 < timCF> And it's coming from here I guess https://github.com/bitcoin-core/secp256k1/blob/0440945fb5ce69d335fed32827b5166e84b02e05/src/secp256k1.c#L380
tim2CF commented 3 years ago
13:59 < tomsmeding> timCF: that mempty, is that a strict bytestring?
14:00 < timCF> Yes, just strict ByteString. Does anybody know how in principle on low value level `Data.ByteString.empty` differs from `mempty :: ByteString` or `"" :: ByteString`
14:00 < tomsmeding> Data.ByteString.empty and mempty :: ByteString are both backed by a null pointer, which is passed unchanged to C apis
14:00 < tomsmeding> E.g. empty: https://hackage.haskell.org/package/bytestring-0.11.1.0/docs/src/Data.ByteString.html#empty
14:00 < timCF> `"" :: ByteString` and `mempty :: ByteString` behave differently in my case
14:00 -!- jmorris [~jmorris@2001:19f0:5801:170:5400:3ff:fe81:62fe] has quit [Quit: BYE BITCH]
14:01 < tomsmeding> With OverloadedStrings?
14:01 -!- jmorris [~jmorris@2001:19f0:5801:170:5400:3ff:fe81:62fe] has joined #haskell
14:01 < timCF> Yes
14:01 < timCF> `"" :: ByteString` is not causing NULL pointer excepption, but `mempty :: ByteString` and `Data.ByteString.empty` do
14:02 < tomsmeding> fromString "" :: ByteString creates a zero-length allocation, whereas mempty and empty don't do any allocation at all and just return a null bytestring
14:02 < tomsmeding> Sounds like this is precisely your problem :p
14:02 -!- Matthias1 [~Matthias1@cpe-76-170-236-166.socal.res.rr.com] has joined #haskell
14:02 < tomsmeding> I've had trouble with this before: https://github.com/snapframework/snap-core/pull/305
14:03 < timCF> tomsmeding: hmmm so Haskell will recognize ("" :: ByteString) == (mempty :: ByteString) as True? But in reality it's not? :)
14:04 < tomsmeding> timCF: (==) need not be structural equality 
14:04 < tomsmeding> ;)
14:05 < tomsmeding> In this case, for (==), the case that produces that equality is the first line here 
                    https://hackage.haskell.org/package/bytestring-0.11.1.0/docs/src/Data.ByteString.Internal.html#compareBytes
14:05 < timCF> That's scary, haha) Haskell<->C APIs are scary place) It's first time I have a problems like this)
14:05 < tomsmeding> timCF: this sounds like a bug in the haskell wrapper
14:06 < merijn> tomsmeding: tbh, having a "zero-length" allocation sounds plain wrong
14:06 < tomsmeding> In haskell land, fromString "" and empty are semantically equal, so the wrapper must ensure that that is also the case for these potentially abstraction-breaking functions
14:06 < merijn> Sounds like those should be returning NULL too
14:06 < tomsmeding> merijn: possibly, but it shouldn't be observable :p
14:06 < tomsmeding> So that secp haskell lib should do some checking
tomsmeding commented 3 years ago

14:06 < tomsmeding> So that secp haskell lib should do some checking

I didn't mean to sound demeaning in any way!

I do believe that perhaps importSig should ensure that the pointer it passes to ecdsaSignatureParseDer is non-NULL. As I wrote on irc: Data.ByteString.empty and fromString "" :: ByteString are semantically equal in Haskell, so in order to make the API as foolproof as possible, I would say that library functions should preserve that property (regardless of whether it is sensible to pass empty bytestrings to those functions — whatever it should or shouldn't do, it should not crash :) ).

jprupp commented 3 years ago

Fixed in pull request #37.