libuuid clock.txt fd leaks

libuuid clock.txt fd leaks

[Tim Pepper]

libuuid's gen_uuid.c:get_clock() goes to some length to try to reuse
its fd and avoid repeated opens of /var/lib/libuuid/clock.txt and also
to be thread safe.  But the current implementation must lead to fd
leakage in multithreaded applications.  This has been reported
multiple places as the gen_uuid.c file seems to have been reused in
various open source projects.  But I don't see mention of this issue
in the util-linux archives.

Some mentions I've found:
http://sourceforge.net/tracker/index.php?func=detail&aid=3276476&group_id=2406&atid=102406
https://github.com/samuel/python-gearman/pull/3
And it's come into my lap by way of some complicated Java application
I can't claim to understand, but they error on too many open files and
note via lsof that opens of clock.txt grow unbounded during their
runtime.

While googling around I also noticed the C++98 spec (well maybe
anyway...as referenced here:
http://gcc.gnu.org/onlinedocs/gcc-4.6.1/gcc/C_002b_002b98-Thread_002dLocal-Edits.html#C_002b_002b98-Thread_002dLocal-Edits)
includes the idea that things in TLS shouldn't have non-trivial
destructors.  Shows at least that somebody's thought about the
problem.  I'd guess having to have something know to do fclose() /
close() on parts of the TLS counts as non-trivial and shouldn't be
done.

The sourceforge.net link above includes a number of suggested options.
 And there's also just the simple implementation option for
get_clock() that doesn't have the fd as static much less TLS and
opens/closes at each invocation.  That maybe comes with a noticable
performance hit for some types of users and I don't really know the
full list of possible use cases here...do some people really need that
high performance uuid generation?  Either way, the "right" general
solution isn't immediately obvious to me so I thought I'd see what
others here thought...




Tim

Re: libuuid clock.txt fd leaks

[Ted Ts'o]

On Wed, Sep 21, 2011 at 09:58:21AM -0700, Tim Pepper wrote:
> The sourceforge.net link above includes a number of suggested options.
>  And there's also just the simple implementation option for
> get_clock() that doesn't have the fd as static much less TLS and
> opens/closes at each invocation.  That maybe comes with a noticable
> performance hit for some types of users and I don't really know the
> full list of possible use cases here...do some people really need that
> high performance uuid generation?  Either way, the "right" general
> solution isn't immediately obvious to me so I thought I'd see what
> others here thought...

Part of the problem is that the uuid library was originally designed
non-thread-using libraries, and I originally was trying to force all
programs to link against -lpthread.  (Remember, libuuid as found in
e2fsprogs predates Linux having a standardized threads support, or any
threads support at all.)

Personally, I think threads are evil, and avoid them like the plague,
so I'm not up on the latest threads requirements, but at least at one
point, if a shared library used threads all of the programs that
called said shared library had to link against pthreads and #include
pthread.h, and have errno redefined, etc.  (Remember we need to worry
about making things work both for the static and shared linking case.)

So the reason why I put in THREAD_LOCAL was it was the simplest way to
avoid problems without having access to mutexes and without having to
drag in the whole posix threads mess and inflicting it on all of
libuuid's callers.

Unfortunately, it didn't occur to me that people would write programs
where they spawned jillions of threads and then abandon them, causing
the fd leak that would eventually kill the program.  (See my belief
that Posix Threads are evil.)

It may be that the right answer is to create a separate library which
is used for threading programs --- or develop a completely thread-safe
API for threaded progams, and keep the old API for
sane^H^H^H^Hsingle-threaded programs.

							- Ted

Re: libuuid clock.txt fd leaks

[Ted Ts'o]

On Wed, Sep 21, 2011 at 04:26:20PM -0400, Ted Ts'o wrote:
> Part of the problem is that the uuid library was originally designed
> non-thread-using libraries, and I originally was trying to force all

Sigh, s/to force/to avoid forcing/

> programs to link against -lpthread.  (Remember, libuuid as found in
> e2fsprogs predates Linux having a standardized threads support, or any
> threads support at all.)

						- Ted

Re: libuuid clock.txt fd leaks

[Tim Pepper]

On Wed, Sep 21, 2011 at 1:26 PM, Ted Ts'o <tytso@mit.edu> wrote:
> It may be that the right answer is to create a separate library which
> is used for threading programs --- or develop a completely thread-safe
> API for threaded progams, and keep the old API for
> sane^H^H^H^Hsingle-threaded programs.

I don't think this is so much about API.  The documented behavior for
uuid_generate() and its siblings does not necessarily need to change here.
I view this mostly as an internal implementation detail.

The existing code optimizes to reuse the fd and avoids repeated
open/close calls for a given caller.  If the variables are static,
they're not thread safe.  So they became thread local.  At that point
"interesting" threads may become a problem because of the lack of
destructor to do closes.  But...

Is the original optimization really needed?  If not you avoid the whole
problem...no static, no TLS, no fd left open.

And it looks like a similar thing happens in get_random_fd(), fwiw.

I wonder who would whinge on performance if the open/close simply
happened everytime?  Ie: remove initial state_fd=-2 and its associated
special casing.



Tim

Re: libuuid clock.txt fd leaks

[Tim Pepper]

On Wed 21 Sep at 15:02:48 -0700 tpepper@gmail.com said:
> 
> I wonder who would whinge on performance if the open/close simply
> happened everytime?  Ie: remove initial state_fd=-2 and its associated
> special casing.

Something sort of like this for get_clock(), completely untested...

-- 
Tim Pepper  <lnxninja@linux.vnet.ibm.com>
IBM Linux Technology Center


--- gen_uuid.c.orig	2011-09-16 14:14:58.902434595 -0700
+++ gen_uuid.c	2011-09-21 15:06:35.271419101 -0700
@@ -316,57 +316,48 @@ static int get_node_id(unsigned char *no
 static int get_clock(uint32_t *clock_high, uint32_t *clock_low,
 		     uint16_t *ret_clock_seq, int *num)
 {
-	THREAD_LOCAL int		adjustment = 0;
-	THREAD_LOCAL struct timeval	last = {0, 0};
-	THREAD_LOCAL int		state_fd = -2;
-	THREAD_LOCAL FILE		*state_f;
-	THREAD_LOCAL uint16_t		clock_seq;
+	int				adjustment = 0;
+	struct timeval			last = {0, 0};
+	int				fd;
+	FILE				*f;
+	uint16_t			clock_seq;
 	struct timeval			tv;
 	uint64_t			clock_reg;
 	mode_t				save_umask;
 	int				len;
 	int				ret = 0;
-
-	if (state_fd == -2) {
-		save_umask = umask(0);
-		state_fd = open("/var/lib/libuuid/clock.txt",
-				O_RDWR|O_CREAT, 0660);
-		(void) umask(save_umask);
-		if (state_fd != -1) {
-			state_f = fdopen(state_fd, "r+");
-			if (!state_f) {
-				close(state_fd);
-				state_fd = -1;
-				ret = -1;
-			}
+	unsigned int			cl;
+	unsigned long			tv1, tv2;
+	int				a;
+
+	save_umask = umask(0);
+	fd = open("/var/lib/libuuid/clock.txt",
+			O_RDWR|O_CREAT, 0660);
+	(void) umask(save_umask);
+	if (fd != -1) {
+		f = fdopen(fd, "r+");
+		if (!f) {
+			close(fd);
+			return -1;
 		}
-		else
-			ret = -1;
 	}
-	if (state_fd >= 0) {
-		rewind(state_f);
-		while (flock(state_fd, LOCK_EX) < 0) {
-			if ((errno == EAGAIN) || (errno == EINTR))
-				continue;
-			fclose(state_f);
-			close(state_fd);
-			state_fd = -1;
-			ret = -1;
-			break;
-		}
+	else
+		return -1;
+
+	while (flock(fd, LOCK_EX) < 0) {
+		if ((errno == EAGAIN) || (errno == EINTR))
+			continue;
+		fclose(f);
+		close(fd);
+		return -1;
 	}
-	if (state_fd >= 0) {
-		unsigned int cl;
-		unsigned long tv1, tv2;
-		int a;
-
-		if (fscanf(state_f, "clock: %04x tv: %lu %lu adj: %d\n",
-			   &cl, &tv1, &tv2, &a) == 4) {
-			clock_seq = cl & 0x3FFF;
-			last.tv_sec = tv1;
-			last.tv_usec = tv2;
-			adjustment = a;
-		}
+
+	if (fscanf(f, "clock: %04x tv: %lu %lu adj: %d\n",
+		   &cl, &tv1, &tv2, &a) == 4) {
+		clock_seq = cl & 0x3FFF;
+		last.tv_sec = tv1;
+		last.tv_usec = tv2;
+		adjustment = a;
 	}
 
 	if ((last.tv_sec == 0) && (last.tv_usec == 0)) {
@@ -406,19 +397,18 @@ try_again:
 		last.tv_usec = last.tv_usec % 1000000;
 	}
 
-	if (state_fd >= 0) {
-		rewind(state_f);
-		len = fprintf(state_f,
-			      "clock: %04x tv: %016lu %08lu adj: %08d\n",
-			      clock_seq, last.tv_sec, last.tv_usec, adjustment);
-		fflush(state_f);
-		if (ftruncate(state_fd, len) < 0) {
-			fprintf(state_f, "                   \n");
-			fflush(state_f);
-		}
-		rewind(state_f);
-		flock(state_fd, LOCK_UN);
-	}
+	rewind(f);
+	len = fprintf(f,
+		      "clock: %04x tv: %016lu %08lu adj: %08d\n",
+		      clock_seq, last.tv_sec, last.tv_usec, adjustment);
+	fflush(f);
+	if (ftruncate(fd, len) < 0) {
+		fprintf(f, "                   \n");
+		fflush(f);
+	}
+	fclose(f);
+	flock(fd, LOCK_UN);
+	close(fd);
 
 	*clock_high = clock_reg >> 32;
 	*clock_low = clock_reg;

Re: libuuid clock.txt fd leaks

[Ted Ts'o]

On Wed, Sep 21, 2011 at 03:02:48PM -0700, Tim Pepper wrote:
> Is the original optimization really needed?  If not you avoid the whole
> problem...no static, no TLS, no fd left open.

> I wonder who would whinge on performance if the open/close simply
> happened everytime?  Ie: remove initial state_fd=-2 and its associated
> special casing.

There are programs out there that generate literally tens of thousands
of UUID's per second.  One example of such a program filed high
priority bugs with both RHEL and SLES because it wasn't fast enough
and the uuid library was slowing down the setup time of an
commercially important Enterprise Resource Planning product by hours.

The reason why I implemented uuidd was specifically to help out this
product and this company.  Partially because it was economically
important to Red Hat, SLES, and IBM (who was my employer at the time),
and partially because I had a soft spot for this company, to which I had
helped add Kerberos authentication and was very supportive of Linux in
the mid 1990's, before Linux had been discovered by big business.

But yeah, there are people and companies who really care about
libuuid's performance.

    	  	    	       	      	   - Ted

Re: libuuid clock.txt fd leaks

[Tim Pepper]

On Wed 21 Sep at 18:24:12 -0400 tytso@mit.edu said:
> 
> There are programs out there that generate literally tens of thousands
> of UUID's per second.

That's what I worried.  I figured you wouldn't have done this if that
weren't the case.  One can hope though right?  Perhaps for the use case
I'm looking at they don't need performance as much as they need the fd
not to leak.  We'll see...

-- 
Tim Pepper  <lnxninja@linux.vnet.ibm.com>
IBM Linux Technology Center