crypt mutex

crypt mutex

[Kevin Ryde]

[-- Attachment #1: Type: text/plain, Size: 281 bytes --]

While nosing around the crypt function, I wondered if it ought to have
a mutex, just in case two threads run it concurrently.

        * posix.c (crypt_mutex): New variable.
        (scm_init_posix): Initialize it.
        (scm_crypt): Use it to protect static data from crypt().


[-- Attachment #2: posix.c.crypt-mutex.diff --]
[-- Type: text/plain, Size: 1210 bytes --]

--- posix.c.~1.124.~	2004-02-21 09:19:52.000000000 +1000
+++ posix.c	2004-02-21 10:30:54.000000000 +1000
@@ -1420,7 +1420,11 @@
 #undef FUNC_NAME
 #endif /* HAVE_SYNC */
 
+/* crypt() returns a pointer to a static buffer, so we use a mutex to avoid
+   another thread overwriting that buffer by another call.
+   OPTIMIZE-ME: Use glibc crypt_r() when available.  */
 #if HAVE_CRYPT
+static scm_t_mutex  crypt_mutex;
 SCM_DEFINE (scm_crypt, "crypt", 2, 0, 0, 
             (SCM key, SCM salt),
 	    "Encrypt @var{key} using @var{salt} as the salt value to the\n"
@@ -1428,12 +1432,16 @@
 #define FUNC_NAME s_scm_crypt
 {
   char * p;
+  SCM ret;
 
   SCM_VALIDATE_STRING (1, key);
   SCM_VALIDATE_STRING (2, salt);
 
+  scm_mutex_lock (&crypt_mutex);
   p = crypt (SCM_STRING_CHARS (key), SCM_STRING_CHARS (salt));
-  return scm_makfrom0str (p);
+  ret = scm_makfrom0str (p);
+  scm_mutex_unlock (&crypt_mutex);
+  return ret;
 }
 #undef FUNC_NAME
 #endif /* HAVE_CRYPT */
@@ -1826,6 +1834,10 @@
   scm_c_define ("LOCK_NB", SCM_MAKINUM (LOCK_NB));
 #endif
 
+#if HAVE_CRYPT
+  scm_mutex_init (&crypt_mutex, &scm_i_plugin_mutex);
+#endif
+
 #include "libguile/cpp_sig_symbols.c"
 #include "libguile/posix.x"
 }

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/guile-devel

Re: crypt mutex

[Mikael Djurfeldt]

Kevin Ryde <user42@zip.com.au> writes:

> While nosing around the crypt function, I wondered if it ought to have
> a mutex, just in case two threads run it concurrently.

We probably need a policy which regulates when to have and when to not
have a mutex.

Personally, I wouldn't like Guile to have everything thread-safe
"under the hood".  That would be a terrible waste of resources.

I think it is completely OK to specify that certain resources are not
thread safe, and that if a user writes a threaded program which uses
such resources in parallel, he needs to himself supply necessary
mutexes etc.

So, the policy needs to specify where the border between Guile and
user responsibility goes.  In most cases, I would probably draw the
line so that as much as possible of the responsibility is left to the
user with the exceptions that 1. Guile should never segfault due to
misuse in this respect, and, 2. Guile need to have enough thread
safety so that it's reasonably convenient to write parallel programs.

M


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/guile-devel

Re: crypt mutex

[Mikael Djurfeldt]

Mikael Djurfeldt <djurfeldt@nada.kth.se> writes:

> So, the policy needs to specify where the border between Guile and
> user responsibility goes.  In most cases, I would probably draw the
> line so that as much as possible of the responsibility is left to the
> user with the exceptions that 1. Guile should never segfault due to
> misuse in this respect, and, 2. Guile need to have enough thread
> safety so that it's reasonably convenient to write parallel programs.

BTW, applying this policy on crypt probably means that we shouldn't
protect it with a mutex.

M


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/guile-devel

Re: crypt mutex

[Kevin Ryde]

Mikael Djurfeldt <djurfeldt@nada.kth.se> writes:
>
> Personally, I wouldn't like Guile to have everything thread-safe
> "under the hood".  That would be a terrible waste of resources.

I suppose for a start reentrant versions of things can be used when
available,

#if HAVE_CRYPT_R
  {
    /* FIXME: In glibc 2.3.2, crypt_data is about 128kbytes, maybe we should
       think about mallocing it so as not to abuse the stack. */
    struct crypt_data data;
    data.initialized = 0;
    return scm_makfrom0str
      (crypt_r (SCM_STRING_CHARS (key), SCM_STRING_CHARS (salt), &data));
  }
#else
  return scm_makfrom0str
    (crypt (SCM_STRING_CHARS (key), SCM_STRING_CHARS (salt)));
#endif


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/guile-devel

Re: crypt mutex

[Marius Vollmer]

Mikael Djurfeldt <djurfeldt@nada.kth.se> writes:

> Kevin Ryde <user42@zip.com.au> writes:
>
>> While nosing around the crypt function, I wondered if it ought to have
>> a mutex, just in case two threads run it concurrently.
>
> We probably need a policy which regulates when to have and when to not
> have a mutex.
>
> Personally, I wouldn't like Guile to have everything thread-safe
> "under the hood".  That would be a terrible waste of resources.

Yes.  Also, what does 'thread-safe' mean, aynway?  (This question is
probably just a nother way to ask what you are asking...)  For
example, can lists be meaningfully made thread safe?

> [...]
>
> So, the policy needs to specify where the border between Guile and
> user responsibility goes.  In most cases, I would probably draw the
> line so that as much as possible of the responsibility is left to the
> user with the exceptions that 1. Guile should never segfault due to
> misuse in this respect, and, 2. Guile need to have enough thread
> safety so that it's reasonably convenient to write parallel programs.

Yes, exactly my view.  Also, I would broaden point 1 a bit: we should
also 'fix' functions that can not every be used in a threaded program
without mutexes around them.  Like libc getpwent.  They might not
segfault, but you can't use them anyway in a threaded program.

-- 
GPG: D5D4E405 - 2F9B BCCC 8527 692A 04E3  331E FAF8 226A D5D4 E405


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/guile-devel

Re: crypt mutex

[Marius Vollmer]

Mikael Djurfeldt <djurfeldt@nada.kth.se> writes:

> BTW, applying this policy on crypt probably means that we shouldn't
> protect it with a mutex.

I'd say we should since it uses a static buffer.  There is no way to
use crypt in a threaded program without putting mutexes around it.

-- 
GPG: D5D4E405 - 2F9B BCCC 8527 692A 04E3  331E FAF8 226A D5D4 E405


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/guile-devel

Re: crypt mutex

[Marius Vollmer]

Kevin Ryde <user42@zip.com.au> writes:

> Mikael Djurfeldt <djurfeldt@nada.kth.se> writes:
>>
>> Personally, I wouldn't like Guile to have everything thread-safe
>> "under the hood".  That would be a terrible waste of resources.
>
> I suppose for a start reentrant versions of things can be used when
> available,

Yes, that is of course better than using a mutex.

-- 
GPG: D5D4E405 - 2F9B BCCC 8527 692A 04E3  331E FAF8 226A D5D4 E405


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/guile-devel

Re: crypt mutex

[Mikael Djurfeldt]

Marius Vollmer <mvo@zagadka.de> writes:

>> In most cases, I would probably draw the line so that as much as
>> possible of the responsibility is left to the user with the
>> exceptions that 1. Guile should never segfault due to misuse in
>> this respect, and, 2. Guile need to have enough thread safety so
>> that it's reasonably convenient to write parallel programs.
>
> Yes, exactly my view.  Also, I would broaden point 1 a bit: we should
> also 'fix' functions that can not every be used in a threaded program
> without mutexes around them.  Like libc getpwent.  They might not
> segfault, but you can't use them anyway in a threaded program.

But the normal case is *not* a threaded program.  The everyday program
can use crypt with a static buffer without mutexes.  A *threaded*
program needs mutexes...

This is why I'm leaning towards a minimal policy---to design for the
common case of non-threaded programs, but leave the possibility open
to write parallel code without too much difficulty.

M


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/guile-devel

Re: crypt mutex

[Mikael Djurfeldt]

Mikael Djurfeldt <mdj@mit.edu> writes:

> Marius Vollmer <mvo@zagadka.de> writes:
>
>>> In most cases, I would probably draw the line so that as much as
>>> possible of the responsibility is left to the user with the
>>> exceptions that 1. Guile should never segfault due to misuse in
>>> this respect, and, 2. Guile need to have enough thread safety so
>>> that it's reasonably convenient to write parallel programs.
>>
>> Yes, exactly my view.  Also, I would broaden point 1 a bit: we should
>> also 'fix' functions that can not every be used in a threaded program
>> without mutexes around them.  Like libc getpwent.  They might not
>> segfault, but you can't use them anyway in a threaded program.
>
> But the normal case is *not* a threaded program.  The everyday program
> can use crypt with a static buffer without mutexes.  A *threaded*
> program needs mutexes...
>
> This is why I'm leaning towards a minimal policy---to design for the
> common case of non-threaded programs, but leave the possibility open
> to write parallel code without too much difficulty.

Of course: If you by 'fix' mean making functions reentrant (that is:
fixes without too much overhead), then I would agree.

M



_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/guile-devel

Re: crypt mutex

[Mikael Djurfeldt]

Marius Vollmer <mvo@zagadka.de> writes:

> can lists be meaningfully made thread safe?

Your general question is very relevant.  Regarding this example, my
answer would be that we should limit ourselves to making sure that we
don't get segfault or memory corruption if multiple threads randomly
mutates pairs.  If a threaded program needs thread safe (in this case:
mutually exclusive) access to a list data structure, it uses a mutex.

M



_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/guile-devel

Re: crypt mutex

[Andreas Voegele]

Mikael Djurfeldt <mdj@mit.edu> writes:

> Mikael Djurfeldt <mdj@mit.edu> writes:
>
>> Marius Vollmer <mvo@zagadka.de> writes:
>>
>>>> In most cases, I would probably draw the line so that as much as
>>>> possible of the responsibility is left to the user with the
>>>> exceptions that 1. Guile should never segfault due to misuse in
>>>> this respect, and, 2. Guile need to have enough thread safety so
>>>> that it's reasonably convenient to write parallel programs.
>>>
>>> Yes, exactly my view.  Also, I would broaden point 1 a bit: we should
>>> also 'fix' functions that can not every be used in a threaded program
>>> without mutexes around them.  Like libc getpwent.  They might not
>>> segfault, but you can't use them anyway in a threaded program.
>>
>> But the normal case is *not* a threaded program.  The everyday program
>> can use crypt with a static buffer without mutexes.  A *threaded*
>> program needs mutexes...
>>
>> This is why I'm leaning towards a minimal policy---to design for the
>> common case of non-threaded programs, but leave the possibility open
>> to write parallel code without too much difficulty.
>
> Of course: If you by 'fix' mean making functions reentrant (that is:
> fixes without too much overhead), then I would agree.

I'd also prefer a minimal policy.  But what do you do if operating
system A provides the reentrant function foo_r() while operating
system B provides foo() only?  The Scheme procedure "foo" should
behave the same on both systems.

I think that there are four options:

1. Use foo_r() if available, otherwise protect foo() with a mutex
   internally.

2. Use foo() and tell the user to protect the Scheme procedure in
   threaded programs.  It doesn't make sense to use foo_r() in this
   scenario.

3. Provide two Scheme procedures: "foo" and a thread safe version
   "foo-r".  "foo-r" uses foo() and a mutex if foo_r() isn't
   available.

4. Provide two modules, a normal and a thread safe version.  A command
   line switch could be used to request thread safety.  IMHO this
   would be useful for stuff like the POSIX and networking procedures.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/guile-devel

Re: crypt mutex

[Mikael Djurfeldt]

Andreas Voegele <voegelas@gmx.net> writes:

> I'd also prefer a minimal policy.  But what do you do if operating
> system A provides the reentrant function foo_r() while operating
> system B provides foo() only?  The Scheme procedure "foo" should
> behave the same on both systems.
>
> I think that there are four options:
>
> 1. Use foo_r() if available, otherwise protect foo() with a mutex
>    internally.
>
> 2. Use foo() and tell the user to protect the Scheme procedure in
>    threaded programs.  It doesn't make sense to use foo_r() in this
>    scenario.
>
> 3. Provide two Scheme procedures: "foo" and a thread safe version
>    "foo-r".  "foo-r" uses foo() and a mutex if foo_r() isn't
>    available.
>
> 4. Provide two modules, a normal and a thread safe version.  A command
>    line switch could be used to request thread safety.  IMHO this
>    would be useful for stuff like the POSIX and networking procedures.

Of these options I think 3 and 4 would add more complexity than is
payed for, and prefer 1 above 2 if foo_r isn't too uncommon.

M


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/guile-devel

Re: crypt mutex

[Marius Vollmer]

Mikael Djurfeldt <mdj@mit.edu> writes:

>> This is why I'm leaning towards a minimal policy---to design for the
>> common case of non-threaded programs, but leave the possibility open
>> to write parallel code without too much difficulty.
>
> Of course: If you by 'fix' mean making functions reentrant (that is:
> fixes without too much overhead), then I would agree.

Yes, that is what I meant.  But, if making a function be reentrant
takes a mutex then we should not hesitate to use one.

-- 
GPG: D5D4E405 - 2F9B BCCC 8527 692A 04E3  331E FAF8 226A D5D4 E405


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/guile-devel

Re: crypt mutex

[Kevin Ryde]

Marius Vollmer <mvo@zagadka.de> writes:
>
> Yes, that is what I meant.  But, if making a function be reentrant
> takes a mutex then we should not hesitate to use one.

Speaking of mutexes, the reason I haven't checked in what I posted is
that I'm wondering if crypt should have its own private mutex.

A mutex for every function seems a bit wasteful, even if it would give
maximum parallelism.  I'm thinking crypt is cpu-only and not often
run, so might as well share a single global mutex.

Incidentally, crypt_r measures significantly slower (about 5x) than
crypt, presumably because it does various initializations, so I'm
thinking not to use that.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/guile-devel

Re: crypt mutex

[Kevin Ryde]

[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]

It took a while but I made the change below to protect crypt,
introducing a new scm_i_misc_mutex intended for miscellaneous uses,
like this one, which don't rate highly enough to deserve their own
personal mutex.

        * threads.c, threads.h (scm_i_misc_mutex): New SCM_GLOBAL_MUTEX.
        * posix.c (scm_crypt): Use it to protect static data in crypt().

Thread trouble in the old code can be provoked fairly easily with
something like the following.  It's not the sort of thing that can be
added to a test suite, but at least it shows the change has an effect
:-).


(use-modules (ice-9 threads))

(define k1 "blahblah")
(define s1 "99")
(define e1 (crypt k1 s1))

(define k2 "foobar")
(define s2 "77")
(define e2 (crypt k2 s2))

(begin-thread
 (while #t
   (display "-")
   (let ((e (crypt k1 s1)))
     (if (not (string=? e1 e))
         (begin
           (format #t "oops, wrong e1, got ~s want ~s\n" e e1)
           (exit 1))))))

(while #t
  (display ".")
  (let ((e (crypt k2 s2)))
    (if (not (string=? e2 e))
        (begin
          (format #t "oops, wrong e2, got ~s want ~s\n" e e2)
          (exit 1)))))



[-- Attachment #2: posix.c.crypt.diff --]
[-- Type: text/plain, Size: 2167 bytes --]

--- posix.c.~1.132.~	2004-07-14 10:19:36.000000000 +1000
+++ posix.c	2004-07-23 17:12:23.000000000 +1000
@@ -1426,20 +1426,46 @@
 #undef FUNC_NAME
 #endif /* HAVE_SYNC */
 
+
+/* crypt() returns a pointer to a static buffer, so we use scm_i_misc_mutex
+   to avoid another thread overwriting it.  A test program running crypt
+   continuously in two threads can be quickly seen tripping this problem.
+   crypt() is pretty slow normally, so a mutex shouldn't add much overhead.
+
+   glibc has a thread-safe crypt_r, but (in version 2.3.2) it runs a lot
+   slower (about 5x) than plain crypt if you pass an uninitialized data
+   block each time.  Presumably there's some one-time setups.  The best way
+   to use crypt_r to allow parallel execution in multiple threads would
+   probably be to maintain a little pool of initialized crypt_data
+   structures, take one and use it, then return it to the pool.  That pool
+   could be garbage collected so it didn't add permanently to memory use in
+   case only a few crypt calls are made.  But we expect crypt will be used
+   rarely, and even more rarely will there be any desire for lots of
+   parallel execution on multiple cpus.  So for now we don't bother with
+   anything fancy, just ensure it's thread-safe.  */
+
 #if HAVE_CRYPT
-SCM_DEFINE (scm_crypt, "crypt", 2, 0, 0, 
+SCM_DEFINE (scm_crypt, "crypt", 2, 0, 0,
             (SCM key, SCM salt),
 	    "Encrypt @var{key} using @var{salt} as the salt value to the\n"
 	    "crypt(3) library call.")
 #define FUNC_NAME s_scm_crypt
 {
-  char * p;
-
+  SCM ret;
   SCM_VALIDATE_STRING (1, key);
   SCM_VALIDATE_STRING (2, salt);
 
-  p = crypt (SCM_STRING_CHARS (key), SCM_STRING_CHARS (salt));
-  return scm_makfrom0str (p);
+  scm_frame_begin (0);
+  scm_frame_unwind_handler ((void(*)(void*)) scm_mutex_unlock,
+                            &scm_i_misc_mutex,
+                            SCM_F_WIND_EXPLICITLY);
+  scm_mutex_lock (&scm_i_misc_mutex);
+
+  ret = scm_makfrom0str (crypt (SCM_STRING_CHARS (key),
+                                SCM_STRING_CHARS (salt)));
+
+  scm_frame_end ();
+  return ret;
 }
 #undef FUNC_NAME
 #endif /* HAVE_CRYPT */

[-- Attachment #3: Type: text/plain, Size: 143 bytes --]

_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel