[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: patch for mmap and friends

From: Maxime Devos
Subject: Re: patch for mmap and friends
Date: Sat, 14 Jan 2023 16:18:58 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0

On 14-01-2023 01:49, Matt Wette wrote:
Please consider this patch for adding mmap(), munmap() and msync()
 to libguile/filesys.c.  Included is update for posix.texi and test file mman.test.
Once included, feature 'mman should be #t.


+  if (SCM_UNBNDP (fd))
+    c_fd = -1;
+  else
+    c_fd = scm_to_int (fd);

Port objects should be accepted too, as previously asked on <>. As implied by later comments, using a raw fd causes problems with 'move->fdes'. For the remaining response, I'll assume that the function accepts ports as well.


After this code, the port 'fd' becomes unreferenced by this function.

+  if (SCM_UNBNDP (offset))
+    c_offset = 0;
+  else
+    c_offset = scm_to_off_t (offset);
+  if ((c_addr == NULL) && (c_flags & MAP_FIXED))
+    scm_misc_error ("mmap", "cannot have NULL addr w/ MAP_FIXED", SCM_EOL);

Hence, if the GC is run here, its possible for fd to be automatically closed here.

+  SCM_SYSCALL (c_mem = mmap(c_addr, c_len, c_prot, c_flags, c_fd, c_offset));

As such, C 'mmap' could be called on a closed file descriptor even even if the argument to Scheme 'mmap' was an open port, which isn't going to work.

(While it is recommended for Scheme code to keep a reference to the port to manually close afterwards, to free resources faster than waiting for GC, it is not actually required.)

Worse, if the port is closed (by GC), in the mean time another thread may have opened a new port, whose file descriptor coincides with c_fd.

To avoid this problem, you can add

  scm_remember_upto_here_1 (fd);

after the SCM_SYSCALL.

Even then, a problem remains -- a concurrent thread might be using 'move->fdes' to 'move' the file descriptor, hence invalidating c_fd. Functions like 'scm_seek' handle this with 'scm_dynwind_acquire_port' (IIUC, it takes a mutex to delay concurrent 'move->fdes' until finished).

IIUC, the solution then looks like (ignoring wrapping) (the lack of scm_remember_upto_here_1 is intentional):

scm_dynwind_begin (0);
scm_dynwind_acquire_port (fd); // (accepts both ports and numbers, IIUC)
// needs to be after scm_dynwind_acquire_port
if (SCM_UNBNDP (fd))
  c_fd = -1;
  c_fd = scm_to_int (fd);

SCM_SYSCALL (c_mem = mmap(c_addr, c_len, c_prot, c_flags, c_fd, c_offset));
if (c_mem == MAP_FAILED)
    scm_syserror ("mmap");
scm_dynwind_end ();

(I'm not really familiar with scm_dynwind_begin & friends, I'm mostly copy-pasting from libguile/ports.c here.)

+  if (c_mem == MAP_FAILED)
+    scm_syserror ("mmap");              /* errno set */
+  pointer = scm_cell (scm_tc7_pointer, (scm_t_bits) c_mem);
+  bvec = scm_c_take_typed_bytevector((signed char *) c_mem + c_offset, c_len,
+                                    SCM_ARRAY_ELEMENT_TYPE_VU8, pointer);

+  assert(sizeof(void*) <= sizeof(size_t));

IIRC there is a C trick involving fields, arrays and types to check this at compile-time instead. Maybe:

struct whatever {
   /* if availability of zero-length arrays can be assumed */
   int foo[sizeof(size_t) - sizeof(void*)];
   /* alternatively, a weaker but portable check */
   int foo[sizeof(size_t) - sizeof(void*) + 1];


Attachment: OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]