Opened 8 years ago

Closed 7 years ago

#5664 closed bug (fixed)

Missing checks for FFI declaration types

Reported by: simonmar Owned by: pcapriotti
Priority: normal Milestone: 7.6.1
Component: Compiler (FFI) Version: 7.2.1
Keywords: Cc: dterei
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: GHC accepts invalid program Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

A foreign import "wrapper" should have the form f -> IO (FunPtr f), and a foreign import "dynamic" should have the form FunPtr f -> f. GHC currently lacks these checks; the type argument to the FunPtr is ignored in both cases.

Ian Lynagh's example is below; we should be rejecting mkFun2 and mkCallback2.

module A where

import Foreign
import Foreign.C

data D = D

foreign import ccall "dynamic"
  mkFun1 :: FunPtr (CInt -> IO ()) -> (CInt -> IO ())

foreign import ccall "dynamic"
  mkFun2 :: FunPtr (D -> IO ()) -> (CInt -> IO ())

foreign import ccall "wrapper"
  mkCallBack1 :: IO () -> IO (FunPtr (IO ()))

foreign import ccall "wrapper"
  mkCallBack2 :: IO () -> IO (FunPtr D)

Attachments (1)

5664.patch (3.5 KB) - added by pcapriotti 7 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 8 years ago by dterei

Cc: dterei added

comment:2 Changed 8 years ago by simonpj@…

commit 66b047ee8528860ef4babdbe38cf708a41e3be1e

Author: Simon Peyton Jones <simonpj@microsoft.com>
Date:   Tue Dec 13 13:36:34 2011 +0000

    Towards fixing Trac #5664
    
    This patch makes normaliseFFIType recurse ito the arguments of FunPtr

 compiler/typecheck/TcForeign.lhs |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

comment:3 Changed 7 years ago by pcapriotti

difficulty: Unknown
Owner: set to pcapriotti

comment:4 Changed 7 years ago by pcapriotti

Here is the relevant portion of the FFI specification in the Haskell 2010 report:

Dynamic import.
The type of a dynamic stub has to be of the form (FunPtr ft) -> ft, where ft
may be any foreign type.  As an example, consider

foreign import ccall "dynamic"  
  mkFun :: FunPtr (CInt -> IO ()) -> (CInt -> IO ())

The stub factory mkFun converts any pointer to a C function that gets an
integer value as its only argument and does not have a return value into a
corresponding Haskell function.

Dynamic wrapper.
The type of a wrapper stub has to be of the form ft -> IO (FunPtr ft), where ft
may be any foreign type.  As an example, consider

foreign import ccall "wrapper"  
  mkCallback :: IO () -> IO (FunPtr (IO ()))

The stub factory mkCallback turns any Haskell computation of type IO () into a
C function pointer that can be passed to C routines, which can call back into
the Haskell context by invoking the referenced function.

We are currently checking whether the signature of a dynamic (resp. wrapper) import has a FunPtr argument (resp. result), but not if the wrapped function type is correct.

Changed 7 years ago by pcapriotti

Attachment: 5664.patch added

comment:5 Changed 7 years ago by pcapriotti

Status: newpatch

The attached patch performs the extra check using eqType.

comment:6 Changed 7 years ago by simonpj

Thanks. Looks right to me, but it took me a while to be sure. Suggestions for comments to make it easier:

  • Needs a Note [blah] with an example in isFFIDynTy.
  • Comment on isFFIDynTy mentions Addr but code does not?
  • The comment on isFFIDynTy should say what the two argument types are, linked to your example. It took me a while to understand what the curried_res_ty was doing, until I realised that arg_tys was all the args except the FunPtr.
  • The comment says that the check is modulo newtypes, but splitTyConApp_maybe does not look through newtypes, so I thought it was wrong. But it's ok because we have previously called normaliseFfiType. Worth a comment to point this out, as is done in TcForeign line 366, for example.

Please push, and add a test. Thanks!

Simon

comment:7 Changed 7 years ago by p.capriotti@…

commit 88d61ccd9450ed41b99136269a97b2c118462fa4

Author: Paolo Capriotti <p.capriotti@gmail.com>
Date:   Tue Apr 3 10:41:52 2012 +0100

    Improved checks for "dynamic" and "wrapper" foreign declarations (#5664)

 compiler/typecheck/TcForeign.lhs |    6 ++--
 compiler/typecheck/TcType.lhs    |   43 ++++++++++++++++++++++++++++---------
 2 files changed, 35 insertions(+), 14 deletions(-)

comment:8 in reply to:  6 Changed 7 years ago by pcapriotti

Resolution: fixed
Status: patchclosed

Replying to simonpj:

  • Comment on isFFIDynTy mentions Addr but code does not?

I think Addr is not accepted anymore, so that's just a case of out-of-date comment. I removed mentions of Addr in a separate commit.

Please push, and add a test. Thanks!

Done, thank you for the review!

Note: See TracTickets for help on using tickets.