Improper locking bugs(e.g., resource leak) due to the unreleased lock sleep_mutex

[ycaibb]

HI, it seems that the lock sleep_mutex(the resource) is not correctly released in the thread pthread_create(&thread, &attr, syslog_thread, (void *) &t_arg) ?

unionfs-fuse/src/usyslog.c

Lines 155 to 173 in 30c745c

	static void * syslog_thread(void *arg)
	{
	// FIXME: What is a better way to prevent a compiler warning about
	// unused variable 'arg'
	int tinfo = *((int *) arg);
	if (tinfo == 0 && tinfo == 1)
	printf("Starting thread %d", tinfo);
	pthread_mutex_t sleep_mutex;
	pthread_mutex_init(&sleep_mutex, NULL);
	pthread_mutex_lock(&sleep_mutex);
	while (1) {
	pthread_cond_wait(&cond_message, &sleep_mutex);
	do_syslog();
	}
	return NULL;
	}

unionfs-fuse/src/usyslog.c

Line 295 in 30c745c

int res = pthread_create(&thread, &attr, syslog_thread, (void *) &t_arg);

[bsbernd]

Oh well, clearly a bug I had introduced. Now more than 10 years later I don't even remember what this lock is supposed to be about - the mutex is on the stack and locally only - there is no point for a mutex here at all. The missing unlock also does not matter much, as there is no clean shutdown of this thread - I see this as another bug.

[ycaibb]

@aakefbs OK, thank you very much for clarifying this bug!

[bsbernd]

Ah I see why I had added it - because pthread_cond_wait requires a mutex argument....

[bsbernd]

Well, while looking at the code I see a real bug there - there is a lock order violation of lock_list and log_entry::lock.
I will see if I find the time to fix this during the next days.

[ycaibb]

Yes, there could be

static void do_syslog(void)
{
	pthread_mutex_lock(&list_lock); // we MUST ensure not to keep that forever

	while (log_entry) {
		pthread_mutex_t *entry_lock = &log_entry->lock;
		int res = pthread_mutex_trylock(entry_lock);
		if (res) {
			pthread_mutex_unlock(&list_lock);
			...;
			pthread_mutex_lock(&list_lock);
			continue;
		}
		pthread_mutex_unlock(&list_lock);
		...;
		pthread_mutex_lock(&list_lock);
		...;
		pthread_mutex_unlock(&list_lock); // unlock ist ASAP
		pthread_mutex_unlock(entry_lock);
	}
}

[bsbernd]

@rpodgorny Do you have any objections to add a bit of C++? I'm in the process of rewriting the entire usyslog, but it is all basic list work - with C++ this would be all covered by the standard lib and much less error prone to implement. From my view of 10 years later - I would reject my own code in usyslog.c. So if you would accept C++, I could stop my current rewrite, start from scratch and simplify it a lot, I think.

[rpodgorny]

@aakefbs hmmm, actually i'm quite hesitant in doing so - i am afraid that by requiring a c++ compiler (with its lots-of-memory-required-during-compilation, some-exotic-platforms-not-fully-supported, etc.) we'll be limiting the number of systems that will be able to run unionfs-fuse. i'd like to keep unionfs compatible with even the most exotic embedded systems imaginable...

[bsbernd]

Well, the entire usyslog can be optional - in most cases no need for logging at all. So we can just add compilation flags to disable usyslog. The current bugs in usyslog are a result from trying to make it fast and to avoid blocking while it sends to syslog. This can be easily avoided by doing a list splice - take the list with entries, splice it to a list that has all the syslog work - keeps a lock only during list splice. The syslog work list would be used locally only - no need for a lock at all. Now C++ supports list splice - in C it has to be implemented manually. From experience from the current project I'm working on - we implemented list splice on our own some years ago and introduced two times critical bugs. After the first bug a unit test was added, but as typical for unit tests - it was manually written and didn't catch the 2nd bug.
So I'm more tempted to remove usyslog at all in plain C than to introduce even more bugs... Right now usyslog has

• possible deadlock due to lock order violation
• wrong lock used for the pthread_cond_wait

All my bugs and especially the lock order violation is hard to fix with the current code and the two different locks, without the risk to introduce a stall when syslog would wait for unionfs and unionfs would wait for syslog.

[rpodgorny]

ok, then, i agree... ...so let's make this a two-phase thing: first, let's make usyslog compile-time selectable (with the current buggy implementation). second, if you feel like replacing the current implementation with the (battle tested) c++ version, i'll be happy to include it...

[bsbernd]

I hope I get a patch ready in the next 3 days - I think I found an easy splice way in plain C. Just very hard to find time for it when the main work project/filesystem has ways more serious issues.

[rpodgorny]

I hope I get a patch ready in the next 3 days - I think I found an easy splice way in plain C. Just very hard to find time for it when the main work project/filesystem has ways more serious issues.

don't worry bernd, i'll wait for as long as necessary - contributing to open-source projects should be fun, not an obligation so whenever you're ready... ;-)