[Guile-commits] 01/02: Don't hold lock during scm_async_tick in readdir and ttyname

[Rob Browning]

rlb pushed a commit to branch main
in repository guile.

commit 63756efbc5d015a17627fdc446992fc0f7aa6a49
Author: Rob Browning <rlb@defaultvalue.org>
AuthorDate: Wed Jan 22 12:55:06 2025 -0600

    Don't hold lock during scm_async_tick in readdir and ttyname
    
    Only hold scm_i_misc_mutex while making the C calls.  This also avoids
    the need for a dynamic-wind.  Add SCM_I_LOCKED_SYSCALL (similar to
    SCM_SYSCALL) to handle the locking and EINTR loop.
    
    libguile/filesys.c (scm_readdir): rely on SCM_I_LOCKED_SYSCALL to limit
    locking.
    libguile/filesys.c (scm_ttyname): rely on SCM_I_LOCKED_SYSCALL to limit
    locking.
    libguile/syscalls.h: add SCM_I_LOCKED_SYSCALL.
---
 NEWS                |  2 ++
 libguile/filesys.c  | 32 ++++++++++++--------------------
 libguile/posix.c    | 37 ++++++++++++-------------------------
 libguile/syscalls.h | 18 ++++++++++++++++++
 4 files changed, 44 insertions(+), 45 deletions(-)

diff --git a/NEWS b/NEWS
index 3328a03cf..f042188c1 100644
--- a/NEWS
+++ b/NEWS
@@ -69,6 +69,8 @@ every line in a file.
 ** Immutable stringbufs are now 8-byte aligned on all systems
    Previously they could end up with an alignment that violated the type
    tag for their type (e.g. ending up tagged as immediates SCM_IMP()).
+** `readdir` and `ttyname` now release scm_i_misc_mutex during asyncs
+   This avoids potential deadlocks.
 
 
 Changes in 3.0.10 (since 3.0.9)
diff --git a/libguile/filesys.c b/libguile/filesys.c
index b70fbb1ce..0e7078cf0 100644
--- a/libguile/filesys.c
+++ b/libguile/filesys.c
@@ -2231,30 +2231,22 @@ SCM_DEFINE (scm_readdir, "readdir", 1, 0, 0,
            "end of file object is returned.")
 #define FUNC_NAME s_scm_readdir
 {
-  SCM ret;
-  scm_i_pthread_mutex_t *mutex;
-  struct dirent_or_dirent64 *rdent;
-
   SCM_VALIDATE_DIR (1, port);
   if (!SCM_DIR_OPEN_P (port))
     SCM_MISC_ERROR ("Directory ~S is not open.", scm_list_1 (port));
 
-  mutex = (scm_i_pthread_mutex_t *) SCM_SMOB_DATA_2 (port);
-
-  scm_dynwind_begin (0);
-  scm_i_dynwind_pthread_mutex_lock (mutex);
-
-  errno = 0;
-  SCM_SYSCALL (rdent = readdir_or_readdir64 ((DIR *) SCM_SMOB_DATA_1 (port)));
-  if (errno != 0)
-    SCM_SYSERROR;
-
-  ret = (rdent ? scm_from_locale_stringn (rdent->d_name, NAMLEN (rdent))
-         : SCM_EOF_VAL);
-
-  scm_dynwind_end ();
-
-  return ret;
+  scm_i_pthread_mutex_t *mutex = (scm_i_pthread_mutex_t *) SCM_SMOB_DATA_2 
(port);
+  DIR *dir = (DIR *) SCM_SMOB_DATA_1 (port);
+  char *name = 0;
+  SCM_I_LOCKED_SYSCALL
+    (mutex,
+     struct dirent_or_dirent64 *rdent = readdir_or_readdir64 (dir);
+     if (rdent) name = strdup (rdent->d_name));
+  if (name)
+    return scm_take_locale_string (name);
+  if (!errno)
+    return SCM_EOF_VAL;
+  SCM_SYSERROR;
 }
 #undef FUNC_NAME
 
diff --git a/libguile/posix.c b/libguile/posix.c
index 0e57f012b..61b5523af 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1048,8 +1048,10 @@ SCM_DEFINE (scm_getsid, "getsid", 1, 0, 0,
    continuously calling ttyname will otherwise get an overwrite quite
    easily.
 
-   ttyname_r (when available) could be used instead of scm_i_misc_mutex, but
-   there's probably little to be gained in either speed or parallelism.  */
+   ttyname_r (when available) could be used instead of scm_i_misc_mutex
+   if it doesn't restrict the maximum name length the way readdir_r can,
+   but there's probably little to be gained in either speed or
+   parallelism.  */
 
 #ifdef HAVE_TTYNAME
 SCM_DEFINE (scm_ttyname, "ttyname", 1, 0, 0, 
@@ -1058,34 +1060,19 @@ SCM_DEFINE (scm_ttyname, "ttyname", 1, 0, 0,
            "underlying @var{port}.")
 #define FUNC_NAME s_scm_ttyname
 {
-  char *result;
-  int fd, err;
-  SCM ret = SCM_BOOL_F;
-
   port = SCM_COERCE_OUTPORT (port);
   SCM_VALIDATE_OPPORT (1, port);
   if (!SCM_FPORTP (port))
     return SCM_BOOL_F;
-  fd = SCM_FPORT_FDES (port);
-
-  scm_i_scm_pthread_mutex_lock (&scm_i_misc_mutex);
-
-  SCM_SYSCALL (result = ttyname (fd));
-  err = errno;
-  if (result != NULL)
-    result = strdup (result);
 
-  scm_i_pthread_mutex_unlock (&scm_i_misc_mutex);
-
-  if (!result)
-    {
-      errno = err;
-      SCM_SYSERROR;
-    }
-  else
-    ret = scm_take_locale_string (result);
-
-  return ret;
+  int fd = SCM_FPORT_FDES (port);
+  char *name = 0;
+  SCM_I_LOCKED_SYSCALL(&scm_i_misc_mutex,
+                       char *n = ttyname (fd);
+                       if (n) name = strdup (n));
+  if (name)
+    return scm_take_locale_string (name);
+  SCM_SYSERROR;
 }
 #undef FUNC_NAME
 #endif /* HAVE_TTYNAME */
diff --git a/libguile/syscalls.h b/libguile/syscalls.h
index 6f4061138..e10cc4870 100644
--- a/libguile/syscalls.h
+++ b/libguile/syscalls.h
@@ -37,6 +37,24 @@
     }                                          \
   while (errno == EINTR)
 
+/* A SCM_SYSCALL for calls that need mutex serialization.  The body must
+   ensure to leave an appropriate errno. */
+#define SCM_I_LOCKED_SYSCALL(lock, body)                             \
+  while(1)                                                           \
+    {                                                                \
+      scm_i_pthread_mutex_t *lock___ = (lock);                       \
+      scm_i_pthread_mutex_lock (lock___);                            \
+      errno = 0;                                                     \
+      { body; }                                                      \
+      int err___ = errno;                                            \
+      scm_i_pthread_mutex_unlock (lock___);                          \
+      if (err___ != EINTR)                                           \
+        {                                                            \
+          errno = err___;                                            \
+          break;                                                     \
+        }                                                            \
+      scm_async_tick ();                                             \
+    }