Running 32-bit CCL on 64-bit Linux (but it can happen in any setup), I noticed that (osicat-posix:mmap) does not correctly handle return addresses larger than the maximum C "long" value, but instead throws an exception.
In such cases, it means the underlying C mmap() function returned successfully a pointer in the top half of the addressable virtual memory, but the Lisp wrapper for mmap() and mremap() chokes on such pointers.
A patch that solves this problem is attached.
Feedback on the analysis and the solution is welcome :)
Best regards,
Massimiliano Ghilardi
------------------ longer explanation -----------------------------
While invoking
(osicat-posix:mmap (cffi-sys:null-pointer) 4096 (logior osicat-posix:prot-read osicat-posix:prot-write) osicat-posix:map-shared *fd* 0)
with a file descriptor *fd* correctly open on a 4096-byte file, I encountered the error: ------------------------------------------------------------------- The value -143798272 is not of the expected type (UNSIGNED-BYTE 32). [Condition of type TYPE-ERROR]
Restarts: 0: [RETRY] Retry SLIME REPL evaluation request. 1: [*ABORT] Return to SLIME's top level. 2: [ABORT-BREAK] Reset this thread 3: [ABORT] Kill this thread
Backtrace: 0: (CFFI-SYS:MAKE-POINTER -143798272) 1: (CCL::CALL-CHECK-REGS OSICAT-POSIX:MMAP #<A Null Foreign Pointer> 4096 3 1 4 0) 2: (CCL::CHEAP-EVAL (OSICAT-POSIX:MMAP (CFFI-SYS:NULL-POINTER) 4096 (LOGIOR OSICAT-POSIX:PROT-READ OSICAT-POSIX:PROT-WRITE) OSICAT-POSIX:MAP-SHARED *FD* ...)) -------------------------------------------------------------------
A simple analysis (cat /proc/<ccl-pid>/maps) shows that mmap() actually succeeded and returned the value #xf76dd000, which was converted to -143798272 because OSICAT wrapper for mmap() is defined to return a C "long", that is limited to the range -#x80000000 to #x7fffffff, thus converting #xf76dd000 to a C long overflows and gives a negative value.
But calling (cffi-sys:make-pointer) chokes on negative values, at least on CCL.
On Mon, Jan 6, 2014 at 9:06 PM, Massimiliano Ghilardi massimiliano.ghilardi@gmail.com wrote:
In such cases, it means the underlying C mmap() function returned successfully a pointer in the top half of the addressable virtual memory, but the Lisp wrapper for mmap() and mremap() chokes on such pointers.
A patch that solves this problem is attached.
Feedback on the analysis and the solution is welcome :)
First off, there's another bug: we should be using :intptr rather than :long for the return type. To be fair, CFFI probably didn't have that type yet when that code was written.
The return-filter can then cast the return value to a pointer using something like:
(with-foreign-object (p :intptr) (setf (mem-ref p :intptr) <the-return-value>) (mem-ref p :pointer))
Alternatively, we could store a -1 cast to pointer and compare the return value against that. (Hmm, maybe CFFI should have a cast operation. :-))
I'm not sure any of these alternatives are better than your solution. What do you think?
On 01/06/14 23:25, Luís Oliveira wrote:
On Mon, Jan 6, 2014 at 9:06 PM, Massimiliano Ghilardi massimiliano.ghilardi@gmail.com wrote:
In such cases, it means the underlying C mmap() function returned successfully a pointer in the top half of the addressable virtual memory, but the Lisp wrapper for mmap() and mremap() chokes on such pointers.
A patch that solves this problem is attached.
Feedback on the analysis and the solution is welcome :)
First off, there's another bug: we should be using :intptr rather than :long for the return type. To be fair, CFFI probably didn't have that type yet when that code was written.
I did not know about :intptr. From the cffi/src/types.lisp sources it turns out to be "a signed int as wide as :pointer", so either :intptr or :uintptr are good for this case.
Unluckily, converting a negative :intptr to a :pointer still requires some trickery. My solution was (cffi-sys:make-pointer (logand address +most-positive-pointer+))) but it has the annoyance of computing +most-positive-pointer+ correctly.
On the other hand, your solution
The return-filter can then cast the return value to a pointer using something like:
(with-foreign-object (p :intptr) (setf (mem-ref p :intptr) <the-return-value>) (mem-ref p :pointer))
for each call it allocates (or seems to allocate) memory for the foreign-object P, writes an :intptr to it, only to read it back as a :pointer - a type-punning that even some C programmers frown upon.
Alternatively, we could store a -1 cast to pointer and compare the return value against that. (Hmm, maybe CFFI should have a cast operation. :-))
I was rather thinking about defining some constant as ((void *)-1) (which is actually the definition of MAP_FAILED, at least on some systems) and grovel it, but groveled constants can only be integers :(
So your suggestion to have a Lisp constant "map-failed cast to pointer" sounds nice.
This raises another interesting point: at least on my system, 64-bit Lisps (SBCL, CCL) grovel (cl:defconstant map-failed 18446744073709551615 "mmap: failure") while 32-bit ones (CMUCL, CCL) grovel (cl:defconstant map-failed -1 "mmap: failure")
Quite annoying.
I'm not sure any of these alternatives are better than your solution. What do you think?
The situation is much more "dirty" than I thought initially: 1) on Win64, "long" is smaller than "intptr" - see http://permalink.gmane.org/gmane.lisp.cffi.devel/2112
2) groveled map-failed sometimes is -1 and sometimes is #xFFFF...FFFF
Also, I'd rather minimize consing and thus call (with-foreign-object ...) sparingly. For the same reason, I'd rather minimize the use of :uintptr, which can exceed fixnum and thus allocate memory.
My opinion is:
1) using *signed* integers (of any size) to represent pointers is asking for trouble. Also, cffi-sys:make-pointer does not accept them. So I strongly prefer to use :uintptr as mmap/mremap return type.
2) Thinking that (or actually defining) MAP_FAILED = -1 is misleading. MAP_FAILED is (void *)-1.
3) There is no predefined constant for "the maximum value of a :pointer" or more exactly "the maximum value of an :uintptr". An accurate definition requires groveling CHAR_BIT from <limits.h> and then defining (defconstant +most-positive-uintptr+ (1- (ash 1 (* char-bit (cffi:foreign-type-size :uintptr)))))
alternatively, the type-punning trick you suggested can be used as well (defconstant +most-positive-uintptr+ (with-foreign-object (p :intptr) (setf (mem-ref p :intptr) -1) (mem-ref p :uintptr)))
I'd go for this second option, as it avoids groveling CHAR_BIT.
4) once we have +most-positive-uintptr+, we can easily work around the fact that sometimes groveled map-failed is positive and sometimes not.
In conclusion, I like your ideas and I updated the patch by integrating most of them.
On Thu, Jan 9, 2014 at 12:15 AM, Massimiliano Ghilardi massimiliano.ghilardi@gmail.com wrote:
- Thinking that (or actually defining) MAP_FAILED = -1 is misleading. MAP_FAILED is (void *)-1.
Your patch looks good. Perhaps we could teach cffi-grovel how to grovel pointers, then we'd be able to ditch the *map-failed-pointer* bits. I've attached an untested patch that does that. Does using (constant ... :type pointer) for the MAP_* constants solve the problem you found?
Cheers,
On 01/09/14 02:08, Luís Oliveira wrote:
On Thu, Jan 9, 2014 at 12:15 AM, Massimiliano Ghilardi massimiliano.ghilardi@gmail.com wrote:
- Thinking that (or actually defining) MAP_FAILED = -1 is misleading. MAP_FAILED is (void *)-1.
Your patch looks good.
Thanks :)
Perhaps we could teach cffi-grovel how to grovel pointers, then we'd be able to ditch the *map-failed-pointer* bits. I've attached an untested patch that does that. Does using (constant ... :type pointer) for the MAP_* constants solve the problem you found?
Cheers,
Yes, after applying your patch to cffi_0.11.2 I could drop *map-failed-pointer* and directly use map-failed, provided that osicat/posix/unixint.lisp is updated to contain
(constant (map-failed "MAP_FAILED") :documentation "mmap: failure" :type cffi-grovel::pointer)
On the other hand, (... :type cffi-grovel::pointer) looks a bit ugly to me, as such symbol is not used anywhere else and is not exported by the package cffi-grovel. What about using the existing symbol cffi-sys:foreign-pointer ? It is the actual Lisp type of the constant/parameter being defined, and it's already exported...
Regards,
Massimiliano
On Thu, 2014-01-09 at 01:08 +0000, Luís Oliveira wrote:
On Thu, Jan 9, 2014 at 12:15 AM, Massimiliano Ghilardi massimiliano.ghilardi@gmail.com wrote:
- Thinking that (or actually defining) MAP_FAILED = -1 is misleading. MAP_FAILED is (void *)-1.
Your patch looks good. Perhaps we could teach cffi-grovel how to grovel pointers, then we'd be able to ditch the *map-failed-pointer* bits. I've attached an untested patch that does that. Does using (constant ... :type pointer) for the MAP_* constants solve the problem you found?
Somebody with more free time than me should try to back-port into cffi iolib-grovel, which is cffi-grovel modified to use C++. iolib-grovel has no problems with pointers because it uses C++ polymorphism and its implicit casts for grovelling.
On Sun, Jan 12, 2014 at 12:37 AM, Stelian Ionescu sionescu@cddr.org wrote:
Somebody with more free time than me should try to back-port into cffi iolib-grovel, which is cffi-grovel modified to use C++. iolib-grovel has no problems with pointers because it uses C++ polymorphism and its implicit casts for grovelling.
What's involved in doing that?
On Sun, 2014-01-12 at 12:20 +0000, Luís Oliveira wrote:
On Sun, Jan 12, 2014 at 12:37 AM, Stelian Ionescu sionescu@cddr.org wrote:
Somebody with more free time than me should try to back-port into cffi iolib-grovel, which is cffi-grovel modified to use C++. iolib-grovel has no problems with pointers because it uses C++ polymorphism and its implicit casts for grovelling.
What's involved in doing that?
Renaming lots of symbols and re-applying the fixes that were done in the meanwhile in cffi-grovel.
On 01/09/14 02:08, Luís Oliveira wrote:
On Thu, Jan 9, 2014 at 12:15 AM, Massimiliano Ghilardi massimiliano.ghilardi@gmail.com wrote:
- Thinking that (or actually defining) MAP_FAILED = -1 is misleading. MAP_FAILED is (void *)-1.
Your patch looks good. Perhaps we could teach cffi-grovel how to grovel pointers, then we'd be able to ditch the *map-failed-pointer* bits. I've attached an untested patch that does that. Does using (constant ... :type pointer) for the MAP_* constants solve the problem you found?
Cheers,
Hello Luís,
as highlighted by you and Stelian Ionescu, there is room to improve CFFI with regards to pointers and automatic type discovery.
With regard to Osicat: do I need to do something more for the mmap()/mremap() patch (attached) to be accepted and integrated?
Best wishes,
Massimiliano Ghilardi
On Mon, Jan 20, 2014 at 6:06 PM, Massimiliano Ghilardi massimiliano.ghilardi@gmail.com wrote:
With regard to Osicat: do I need to do something more for the mmap()/mremap() patch (attached) to be accepted and integrated?
I keep getting errors when trying to push to the c-l.net repository so I took the opportunity to move osicat's repo to github. I've applied your patch there: https://github.com/osicat/osicat.
Cheers,
-
Luís Oliveira -
Massimiliano Ghilardi -
Stelian Ionescu