Improper locking bugs(e.g., deadlocks) on the lock

[ryancaicse]

Hi, developers, thank you for your checking. It seems the lock fd->refs->lock is not released correctly when start < 1 in the function cram_get_ref?

htslib/cram/cram_io.c

Lines 3419 to 3443 in 575db47

	pthread_mutex_lock(&fd->refs->lock);
	if (r->length == 0) {
	if (cram_populate_ref(fd, id, r) == -1) {
	hts_log_error("Failed to populate reference for id %d", id);
	pthread_mutex_unlock(&fd->refs->lock);
	pthread_mutex_unlock(&fd->ref_lock);
	return NULL;
	}
	r = fd->refs->ref_id[id];
	if (fd->unsorted)
	cram_ref_incr_locked(fd->refs, id);
	}
	/*
	* We now know that we the filename containing the reference, so check
	* for limits. If it's over half the reference we'll load all of it in
	* memory as this will speed up subsequent calls.
	*/
	if (end < 1)
	end = r->length;
	if (end >= r->length)
	end = r->length;
	if (start < 1)
	return NULL;

Best,

[ryancaicse]

Also, for the lock fd->ref_lock

htslib/cram/cram_io.c

Line 3375 in 575db47

pthread_mutex_lock(&fd->ref_lock);

[jkbonfield]

Thanks for the bug report. It looks like a real issue.

I'm curious if you tripped over this with real data. I don't believe start should ever be < 1 on valid files, which is likely why we've never triggered it, but as we have an error case where we bail out here then we should also free the locks.

Edit: I see start isn't modified, so we could just move the check to the top along side id == -1 and return before we've done any locking. Seems like a simple fix.

[ryancaicse]

@jkbonfield Thanks.
They are found by code scan. Yes, it looks like, the programs come to the corner cases where the locks are not released properly.

[daviesrob]

@ryancaicse Out of interest, what did you use to find this?

[ryancaicse]

@daviesrob Thank you for your interest. I used Pinpoint. Its code technique could be found here.

[daviesrob]

Thanks, I'll take a look.