Bug 832173 - localtime and ctime are not MT-SAFE
localtime and ctime are not MT-SAFE
Product: GlusterFS
Classification: Community
Component: core (Show other bugs)
All All
high Severity medium
: ---
: ---
Assigned To: Kaleb KEITHLEY
M S Vishwanath Bhat
Depends On:
  Show dependency treegraph
Reported: 2012-06-14 14:13 EDT by Kaleb KEITHLEY
Modified: 2016-05-31 21:56 EDT (History)
4 users (show)

See Also:
Fixed In Version: glusterfs-3.4.0
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2013-07-24 14:01:06 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Attachments (Terms of Use)

  None (edit)
Description Kaleb KEITHLEY 2012-06-14 14:13:06 EDT
Description of problem:

In (heavily) threaded programs like gluster{d,fs,fsd,shd} the use of non-MT-SAFE library functions may result in strange behavior at best, or random, difficult to debug crashes in the worst case.

See http://review.gluster.com/3568 for patches to address the bugs.

There are a number of nit-level issues throughout the source with
the use of localtime and ctime. While they apparently aren't causing
too many problems, apart from the one in bz 828058, they ought to be
fixed. Among the "real" problems that are fixed in the above patch:
1) generally localtime() and ctime() not MT-SAFE. There's a non-zero chance
that another thread calling localtime (or ctime) will over-write
the static data about to be used in another thread
2) localtime(& <64-bit-type>) or ctime(& <64-bit-type>) generally
not a problem on 64-bit or little-endian 32-bit. But even though
we probably have zero users on big-ending 32-bit platforms, it's
still incorrect.
3) multiple nested calls passed as params. Last one wins, i.e. over-
writes result of prior calls.
4) Inconsistent error handling. Most of these calls are for logging,
tracing, or dumping. I submit that if an error somehow occurs in
the call to localtime or ctime, the log/trace/dump still should
still occur.

Other things the above patch fixes/changes (that aren't bugs per se):
1) Change "%Y-%m-%d %H:%M:%S" and similar to their equivalent shorthand,
e.g. "%F %T"
2) change sizeof(timestr) to sizeof timestr. sizeof is an operator,
not a function. You don't use i +(32), why use sizeof(<var>).
(And yes, you do use parens with sizeof(<type>).)
3) change 'char timestr[256]' to 'char timestr[32]' where appropriate.
Per-thread stack is limited. Time strings are never longer than ~20
characters, so why waste 220+ bytes on the stack?

Things the patch doesn't fix:
1) hodgepodge of %Y-%m-%d %H:%M:%S versus %Y/%m/%d-%H%M%S and other
variations. It's not clear to me whether this ever matters, not to
mention 3rd party log filtering tools may already rely on a
particular format. Still it would be nice to have a single manifest
constant and have every call to localtime/strftime consistently use
the same format.

Finally I will assert that appliances, including storage appliances based
on gluster, should all have their clocks set to UTC, and all log entries,
traces, and dumps should use GMT. As such the wide use of localtime_r()
(or localtime()) seems, to me, to be generally incorrect; I'd really like
to see gmtime_r() used everywhere.

Version-Release number of selected component (if applicable):

GlusterFS-3.3.0/RHS 2.0

How reproducible:

Inspection of source.

Steps to Reproduce:
1. N/A
Actual results:


Expected results:


Additional info:  http://review.gluster.com/3568
Comment 1 Kaleb KEITHLEY 2012-06-14 14:20:02 EDT
Equally applicable to 3.2.x
Comment 2 Kaleb KEITHLEY 2012-06-14 15:42:30 EDT
master: http://review.gluster.com/3568

release-3.3: http://review.gluster.com/3572
Comment 3 Amar Tumballi 2012-06-14 23:18:44 EDT
patches sent by kaleb.
Comment 4 Sudhir D 2012-07-09 09:17:34 EDT
Amar can you confirm if comment #2 has the same patches that you mentioned in comment #3 ?
Comment 5 Kaleb KEITHLEY 2012-07-09 09:35:22 EDT
The patch for 3.3 (http://review.gluster.com/3572) has all the same changes/fixes that were in the patch for the head (http://review.gluster.com/3568) including the late fixes for strtok_r().

Is that the question?
Comment 6 Amar Tumballi 2012-07-10 01:53:02 EDT
Sudhir, Yes, Kaleb is right. I was talking about the patches in comment #2 itself.
Comment 7 Vijay Bellur 2013-02-08 15:04:57 EST
CHANGE: http://review.gluster.org/3613 (localtime and ctime are not MT-SAFE) merged in release-3.3 by Anand Avati (avati@redhat.com)

Note You need to log in before you can comment on or make changes to this bug.