Unix.localtime not threadsafe?

Unix.localtime not threadsafe?

[Yaron Minsky]

I was looking at the Unix.localtime implementation, and it appears to
me that it's not threadsafe.  I'm wondering if anyone can confirm.

First off, the underlying localtime call is definitely not re-entrant.
 The tm data structure is shared among all calls, leading to the
possibility of races.  What I'm not sure of is whether there can be a
race given the locking of the OCaml runtime.  Here's the code from
gmtime.c in the ocaml distribution:

static value alloc_tm(struct tm *tm)
{
  value res;
  res = alloc_small(9, 0);
  Field(res,0) = Val_int(tm->tm_sec);
  Field(res,1) = Val_int(tm->tm_min);
  Field(res,2) = Val_int(tm->tm_hour);
  Field(res,3) = Val_int(tm->tm_mday);
  Field(res,4) = Val_int(tm->tm_mon);
  Field(res,5) = Val_int(tm->tm_year);
  Field(res,6) = Val_int(tm->tm_wday);
  Field(res,7) = Val_int(tm->tm_yday);
  Field(res,8) = tm->tm_isdst ? Val_true : Val_false;
  return res;
}

CAMLprim value unix_localtime(value t)
{
  time_t clock;
  struct tm * tm;
  clock = (time_t) Double_val(t);
  tm = localtime(&clock);
  if (tm == NULL) unix_error(EINVAL, "localtime", Nothing);
  return alloc_tm(tm);
}

If I understand the way ocaml's locking works, another thread could
make a call to localtime where alloc_small is called.  If there are
mixed C threads and Ocaml threads, then a call by the C thread at that
point could muck up the tm data structure before switching back to
OCaml, thus leading to bad data getting into the tm data structure.

So, does this seem like a going bug?  We think we may have encountered
this in the wild, so this may be more than a theoretical bug.

Yaron

Re: Unix.localtime not threadsafe?

[Bardur Arantsson]

Yaron Minsky wrote:
> I was looking at the Unix.localtime implementation, and it appears to
> me that it's not threadsafe.  I'm wondering if anyone can confirm.
> 
> First off, the underlying localtime call is definitely not re-entrant.
>  The tm data structure is shared among all calls, leading to the
> possibility of races.  What I'm not sure of is whether there can be a
> race given the locking of the OCaml runtime.  Here's the code from
> gmtime.c in the ocaml distribution:

I don't think the glibc/Linux localtime() man page explicitly states 
this, but I expect that it returns a pointer to a *thread-local* 
statically allocated struct tm... in which case there's no problem.

Most other system functions whose API looks non-threadsafe do the same. 
('errno' would be the standard example).

[--snip--]

-- 
Bardur Arantsson
<bardur@imada.sdu.dk>
<bardur@scientician.net>

- But don't be reading my mind between 4 and 5. That's Willie's
time!
                              Groundskeeper Willie, 'The Simpsons'

Re: [Caml-list] Re: Unix.localtime not threadsafe?

[Florian Weimer]

* Bardur Arantsson:

> I don't think the glibc/Linux localtime() man page explicitly states 
> this, but I expect that it returns a pointer to a *thread-local* 
> statically allocated struct tm... in which case there's no problem.

Thread-local storage is a recent innovation in the Linux camp.
Previously, developers seem to think that no efficient implementation
was possible.  That's why there all the *_r variants (such as
localtime_r).

I believe Solaris does something in the direction you suggest.

Re: [Caml-list] Unix.localtime not threadsafe?

[Xavier Leroy]

> I was looking at the Unix.localtime implementation, and it appears to
> me that it's not threadsafe.  I'm wondering if anyone can confirm.
>
> First off, the underlying localtime call is definitely not re-entrant.
> The tm data structure is shared among all calls, leading to the
> possibility of races.

As others mentioned, the C library implementation of this function
could use thread-local storage to avoid this particular race.  I don't
think this is guaranteed, though.

> If I understand the way ocaml's locking works, another thread could
> make a call to localtime where alloc_small is called.

No, at least not another Caml thread: GC triggered from C code
(e.g. alloc_small()) cannot cause context switches.  So, from a Caml
standpoint, the call to localtime() and the following allocation of
its result are atomic.

> If there are mixed C threads and Ocaml threads, then a call by the C
> thread at that point could muck up the tm data structure before
> switching back to OCaml, thus leading to bad data getting into the
> tm data structure.

Yes, this is possible in principle if 1- some non-Caml threads call
localtime(), and 2- the implementation of that function does not use
thread-local storage.

- Xavier Leroy

Re: [Caml-list] Unix.localtime not threadsafe?

[Yaron Minsky]

On 9/3/05, Xavier Leroy <Xavier.Leroy@inria.fr> wrote:
> > First off, the underlying localtime call is definitely not re-entrant.
> > The tm data structure is shared among all calls, leading to the
> > possibility of races.
> 
> As others mentioned, the C library implementation of this function
> could use thread-local storage to avoid this particular race.  I don't
> think this is guaranteed, though.

Indeed, the man page explicitly states that the function is not
threadsafe.  I think this counts as a bug, and I suspect I may have
seen the bug in the wild.  It seems like it would be simple and safe
to use localtime_r instead.  Should I submit a patch?

> > If I understand the way ocaml's locking works, another thread could
> > make a call to localtime where alloc_small is called.
> 
> No, at least not another Caml thread: GC triggered from C code
> (e.g. alloc_small()) cannot cause context switches.  So, from a Caml
> standpoint, the call to localtime() and the following allocation of
> its result are atomic.
> 
> > If there are mixed C threads and Ocaml threads, then a call by the C
> > thread at that point could muck up the tm data structure before
> > switching back to OCaml, thus leading to bad data getting into the
> > tm data structure.
> 
> Yes, this is possible in principle if 1- some non-Caml threads call
> localtime(), and 2- the implementation of that function does not use
> thread-local storage.

It's interesting to learn that context switches can't happen on c-land
allocations.  That simplifies a lot of things.  But the C-interactions
are still possible, so I would still argue for a fix.  I'm still quite
unsure if the bug I saw is a result of this race, and realizing that
the race can't be between caml-threads makes me more unsure.

Thanks for the help,
Yaron