Improper locking error, unlocking an inactive lock?

[ryancaicse]

Dear developers, thank you for your checking. When Seg_exist(cf, dl_offset)==1, should the unlock statement PTHREAD_MUTEX_UNLOCK(&cf->w_lock); try to unlock a lock cf->w_lock that is not acquired?

According to this,
If a thread attempts to unlock a mutex that it has not locked or a mutex which is unlocked, undefined behavior results.

long Cache_read(Cache *cf, char *const output_buf, const off_t len, const off_t offset_start) {
          if (Seg_exist(cf, dl_offset)) {
                    ...;       // when Seg_exist(cf, dl_offset), it goes to this path
           } else {
                    ...;
                     PTHREAD_MUTEX_LOCK(&cf->w_lock);
           }
         ...;
         PTHREAD_MUTEX_UNLOCK(&cf->w_lock); // unlock a inactive lock
         ...;
 }

httpdirfs/src/cache.c

Lines 995 to 1054 in 939e287

	if (Seg_exist(cf, dl_offset)) {
	send = Data_read(cf, (uint8_t *) output_buf, len, offset_start);
	goto bgdl;
	} else {
	/*
	* Wait for any other download thread to finish
	*/
	lprintf(cache_lock_debug,
	"thread %ld: locking w_lock;\n", pthread_self());
	PTHREAD_MUTEX_LOCK(&cf->w_lock);
	if (Seg_exist(cf, dl_offset)) {
	/*
	* The segment now exists - it was downloaded by another
	* download thread. Send it off and unlock the I/O
	*/
	send =
	Data_read(cf, (uint8_t *) output_buf, len, offset_start);
	lprintf(cache_lock_debug,
	"thread %x: unlocking w_lock;\n", pthread_self());
	PTHREAD_MUTEX_UNLOCK(&cf->w_lock);
	goto bgdl;
	}
	}
	/*
	* ------------------ Download the segment ---------------------
	*/
	uint8_t *recv_buf = CALLOC(cf->blksz, sizeof(uint8_t));
	lprintf(debug, "thread %x: spawned.\n ", pthread_self());
	long recv = Link_download(cf->link, (char *) recv_buf, cf->blksz,
	dl_offset);
	if (recv < 0) {
	lprintf(error, "thread %x received %ld bytes, \
	which does't make sense\n", pthread_self(), recv);
	}
	/*
	* check if we have received enough data, write it to the disk
	*
	* Condition 1: received the exact amount as the segment size.
	* Condition 2: offset is the last segment
	*/
	if ((recv == cf->blksz) ||
	(dl_offset == (cf->content_length / cf->blksz * cf->blksz))) {
	Data_write(cf, recv_buf, recv, dl_offset);
	Seg_set(cf, dl_offset, 1);
	} else {
	lprintf(error, "received %ld rather than %ld, possible network \
	error.\n", recv, cf->blksz);
	}
	FREE(recv_buf);
	send = Data_read(cf, (uint8_t *) output_buf, len, offset_start);
	lprintf(cache_lock_debug,
	"thread %x: unlocking w_lock;\n", pthread_self());
	PTHREAD_MUTEX_UNLOCK(&cf->w_lock);

Best,

[fangfufu]

I do agree this might be a problem. I assumed that the mutex would be locked when the program reaches that point of execution. Feel free to propose a fix.

[fangfufu]

This is definitely quite hard to fix. I had a look today, and I couldn't come up with easy solution. I think the cache system needs a completely rewrite.

[fangfufu]

@jikamens , do you fancy having a look at this?

[jikamens]

I don't understand the problem here. As far as I can tell from the code in Cache_read, when Seg_exist returns true, the code calls goto bgdl, which bypasses the PTHREAD_MUTEX_UNLOCK statement. I.e., the code appears to me to be correct.
I glanced through the rest of the file and I didn't see any obvious instances where an unlock is happening without a corresponding lock.

[fangfufu]

Another friend of mine had a look at this, he said it is okay too.

[fangfufu]

I wonder if this is related:
#126