mmap test doesn't correctly catch error situations

[Christian Kellermann]

I have just came across this issue when dealing with a failed mmap FFI call. In this case mmap (The C function) will return a MAP_FAILED which is defined as (void*)-1 on linux and OpenBSD.

The mmap code checks for this by doing a (eq? -1 addr2). This is fine for 32 bit systems but not for 64 bit systems.

;; on 32 bit
#;2> (address->pointer -1)
#<pointer 0xffffffff>

;; on 64 bit
#;2> (address->pointer -1)
#<pointer 0x0>
#;3> (pointer->address #2)
1.84467440737096e+19

Note how the pointer printing code also gets it wrong. this has been fixed in CHICKEN 5: it will now correctly print the pointer's address, interpreted as an unsigned number.

A workaround would be to explicitly check for the -1 pointer representation:

(pointer=? (address->pointer -1) addr2)

This works but looks icky. I am not sure how to handle this right.

[jrapdx]

Using the mmap API, even in pure C, produced results exactly the same as you note. Here's what I think is going on. Chicken sets the mmap result in the pointer block using the macro C_update_pointer, which simply casts the mmap address to an unsigned int and puts it in the data slot. This is logical--there is no negative machine address.

On error, mmap returns -1 as (unsigned long) (void*), and as unsigned int it's value is 2^{64, not coincidentally represents -1 as a signed int. But the address value is always unsigned, and is not (nor should it be) to "interpret" the mmap error result as a negative number. Thus, the pointer object will sensibly never print an address value less than 0x00, since there is no address less than 0.}

So actually there is nothing wrong with the Chicken code, except for the handling of the mmap error result. However, I can't explain how pointer->address prints the obscure, (double)(unsigned long) (void*) value as an exponential format. Nonetheless, it is the right number, the the max, 2^{64 unsigned long.}

Here's what I'd do to fix it. MMap returns its error as -1, conventional except for the (void*) cast. If not an error, there's no problem, the address will be positive and handled according to design.

A very simple solution would be to check the mmap result (the addr2 variable), Cast the addr2 value to a signed long, and if its -1, check errno, and raise an exception. Wouldn't even bother to convert to an address since it doesn't point anywhere anyway, hence no disturbing output or confusion.

Haven't yet tested it out thoroughly, but I'd wager it works.

JRA

[jrapdx]

The following module implements a plausible way to handle the mmap
error return value within "mmap-file-to-memory". Basically, the
system mmap function return is cast to C_word, defined (in chicken.h)
as a signed 32 or 64 bit value. If -1, an error is raised, otherwise
the return value is cast to an unsigned C_word and then to void*.

This solves the problem of inconsistent 32/64 bit behavior. In
addition, when mmap returns an error result, it seems sensible to
raise an exception rather than simply letting the program crash or
keep going despite illogical, useless (and user-confusing) values.

(module mm (map-file-to-mem)
	(import scheme chicken foreign lolevel)

;; prot: PROT_READ 1, WRITE 2, EXEC 3, NONE 0
;; flag: MAP_FILE 0, ANON #x20, SHARED 1, PRIVATE 2, 
;;       FIXED #x10 (exact addr)

(define mmap-cword 
  (foreign-lambda* long ((c-pointer addr) (integer len) (int prot)
						 (int flag) (int fd) (integer offset))
#<<EOP
   #include <sys/mman.h>
   void *r = mmap (addr, len, prot, flag, fd, offset);
   C_word n = (C_word) r;
   C_return(n);
EOP
))

;; Chicken's 'long' is 32 or 64 bit per system architecture
(define long->ptr (foreign-lambda* c-pointer ((long r))
   "C_return((void*)(C_uword)r);" ))

(define (map-file-to-mem addr len prot flag fd #!optional (off 0))
  (let ((addr-1 (mmap-cword addr len prot flag fd off)))
	(if (= addr-1 -1)
		;; do something with error, eg: (errno is 2 with flag = 0)
		(error (format "mmap-cword errno: ~a (\"~a\")" (errno) 
                   "no such file/directory"))
	    (##sys#make-structure 'mmap (long->ptr addr-1) len))))
)

Though the above "map-file-to-mem" lacks the error checking, etc.,
found in posix-unix.scm, it's functionally a drop-in replacement for
the original procedure.

Running this (modified) excerpt from "posix-tests.scm":

(let ((tnpfilpn (create-temporary-file)))
  (let ((tmpfilno (file-open tnpfilpn (+ open/rdwr open/creat)))
        (data "abcde")
        (size 5))
    (file-write tmpfilno data)
    (let* ((mmap (map-file-to-mem #f size prot/read (+ map/file
													  map/private)
								 tmpfilno))
		   (str (make-string size))
		   (mptr (memory-mapped-file-pointer mmap)))
	  (print "mmap => " mmap "  mptr => " mptr)
      (move-memory! mptr str size)
	  (print "(string=? str data) => " (string=? str data)) 
      (assert (blob=? (string->blob data) (string->blob str)))
      (unmap-file-from-memory mmap))))

prints:

mmap => #<mmap>  mptr => #<pointer 0x7f3160588000>
(string=? str data) => #t

However, with the "map/private" flag removed, mmap -> -1 and the
result is:

Error: mmap-cword errno: 2 ("no such file/directory")

	Call history:

	mmap-test-err.scm:5: load	  
	mmap-test-err.scm:8: print	  
	mmap-test-err.scm:10: create-temporary-file	  
	mmap-test-err.scm:11: file-open	  
	mmap-test-err.scm:14: file-write	  
	mmap-test-err.scm:15: mm#map-file-to-mem	  
	mmap.scm:32: error	  	<--

I hope this idea helps resolve the issue with mmap.

Thanks,
Jules Altfas.

[sjamaan]

(use extras lolevel)
(define minus-1-pointer ((foreign-lambda* c-pointer () "return((void *)-1);")))
(print (number->string (pointer->address (minus-1-pointer)) 16))

This prints 0 in CHICKEN 4, but ffffffffffffffff in CHICKEN 5, so I think this has been fixed.

[sjamaan]

OK, so it's still a bug, the result won't ever be -1, even in the fixed situation (addresses aren't signed quantities!) so the tests need to be adjusted.

[sjamaan]

CHICKEN 5 behaves more sanely, so fixing it there makes more sense to me

[sjamaan]

Actually, it's been taken out of CHICKEN 5 into an egg, so we should fix it there.