bug in mmap() and mremap() return value - and a fix

[Massimiliano Ghilardi]

On 01/06/14 23:25, Luís Oliveira wrote:
> On Mon, Jan 6, 2014 at 9:06 PM, Massimiliano Ghilardi
> <massimiliano.ghilardi at 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.

-------------- next part --------------
diff --git a/posix/early.lisp b/posix/early.lisp
index 88167d7..bf7d5d1 100644
--- a/posix/early.lisp
+++ b/posix/early.lisp
@@ -119,6 +119,13 @@
    (object :initarg :object :reader errno-object)
    (function-name :initarg :function-name :reader function-name)))
 
+;; NOTE: this is an ugly type-punning. An alternative is to compute
+;; this value from (cffi:foreign-type-size :uintptr), but it requires
+;; also knowing how many bits are in a byte.
+(defconstant +most-positive-uintptr+ (with-foreign-object (p :intptr)
+                                       (setf (mem-ref p :intptr) -1)
+                                       (mem-ref p :uintptr)))
+
 ;;; FIXME: undocumented in cffi-grovel.
 (defun make-from-pointer-function-name (type-name)
   (format-symbol t "~A-~A-~A-~A" '#:make type-name '#:from '#:pointer))
diff --git a/posix/unix.lisp b/posix/unix.lisp
index f65ff55..7ddcb2c 100644
--- a/posix/unix.lisp
+++ b/posix/unix.lisp
@@ -308,6 +308,9 @@
 
 ;;;; sys/mman.h
 
+;; on some systems, map-failed is groveled as -1 :(
+(defvar *map-failed-pointer* (make-pointer (logand map-failed +most-positive-uintptr+)))
+
 (defsyscall "mlock" :int
   "Lock a range of process address space."
   (addr :pointer)
diff --git a/posix/wrappers.lisp b/posix/wrappers.lisp
index ae49ad8..dfc2a15 100644
--- a/posix/wrappers.lisp
+++ b/posix/wrappers.lisp
@@ -60,9 +60,8 @@
 
 #-windows
 (defwrapper "mmap" ("void*" (errno-wrapper
-                             :long
-                             :error-predicate (lambda (p) (= p map-failed))
-                             :return-filter make-pointer))
+                             :pointer
+                             :error-predicate (lambda (p) (pointer-eq p *map-failed-pointer*))))
   (start :pointer)
   (length ("size_t" size))
   (prot :int)
@@ -72,9 +71,8 @@
 
 #+linux
 (defwrapper "mremap" ("void*" (errno-wrapper
-                               :long
-                               :error-predicate (lambda (p) (= p map-failed))
-                               :return-filter make-pointer))
+                               :pointer
+                               :error-predicate (lambda (p) (pointer-eq p *map-failed-pointer*))))
   (old-address :pointer)
   (old-size ("size_t" size))
   (new-size ("size_t" size))