Skip to content

Commit

Permalink
Always do the CRAM mutex lock/destroys.
Browse files Browse the repository at this point in the history
For simplicity and to remove a lot of conditionals the code using
mutexes is always used irrespective of whether we are multi-threading.

For example cram_get_ref starts with:

    pthread_mutex_lock(&fd->ref_lock);

The mutexes themselves however are only initialised when using
cram_set_voption with CRAM_OPT_NTHREADS or CRAM_OPT_THREAD_POOL
option.  Attempting to lock an uninitialised mutex has undefined
behaviour.

On linux this does nothing as A) we're not checking the return value
from the lock functions anyway (the only meaningful error is
not-initialised) and B) we calloc the structure and a blank mutex is
the basically the same as initialising it.

Under MSVC however this leads to crashes. So moved both init and
destroy into the cram_dopen function.  Note this adds an additional
destroy which was previously forgotten (fd->range_lock), but again
under linux there is no memory allocated so lacking a destroy will not
leak resources.

Reported by Sebastian Deorowicz
  • Loading branch information
jkbonfield authored and whitwham committed Oct 4, 2023
1 parent 35dd2d0 commit 80ab890
Showing 1 changed file with 10 additions and 12 deletions.
22 changes: 10 additions & 12 deletions cram/cram_io.c
Original file line number Diff line number Diff line change
Expand Up @@ -5349,6 +5349,11 @@ cram_fd *cram_dopen(hFILE *fp, const char *filename, const char *mode) {
fd->ooc = 0;
fd->required_fields = INT_MAX;

pthread_mutex_init(&fd->metrics_lock, NULL);
pthread_mutex_init(&fd->ref_lock, NULL);
pthread_mutex_init(&fd->range_lock, NULL);
pthread_mutex_init(&fd->bam_list_lock, NULL);

for (i = 0; i < DS_END; i++) {
fd->m[i] = cram_new_metrics();
if (!fd->m[i])
Expand Down Expand Up @@ -5546,15 +5551,16 @@ int cram_close(cram_fd *fd) {
if (fd->mode == 'w')
fd->ctr = NULL; // prevent double freeing

pthread_mutex_destroy(&fd->metrics_lock);
pthread_mutex_destroy(&fd->ref_lock);
pthread_mutex_destroy(&fd->bam_list_lock);

//fprintf(stderr, "CRAM: destroy queue %p\n", fd->rqueue);

hts_tpool_process_destroy(fd->rqueue);
}

pthread_mutex_destroy(&fd->metrics_lock);
pthread_mutex_destroy(&fd->ref_lock);
pthread_mutex_destroy(&fd->range_lock);
pthread_mutex_destroy(&fd->bam_list_lock);

if (fd->mode == 'w') {
/* Write EOF block */
if (0 != cram_write_eof_block(fd))
Expand Down Expand Up @@ -5832,10 +5838,6 @@ int cram_set_voption(cram_fd *fd, enum hts_fmt_option opt, va_list args) {
return -1;

fd->rqueue = hts_tpool_process_init(fd->pool, nthreads*2, 0);
pthread_mutex_init(&fd->metrics_lock, NULL);
pthread_mutex_init(&fd->ref_lock, NULL);
pthread_mutex_init(&fd->range_lock, NULL);
pthread_mutex_init(&fd->bam_list_lock, NULL);
fd->shared_ref = 1;
fd->own_pool = 1;
}
Expand All @@ -5849,10 +5851,6 @@ int cram_set_voption(cram_fd *fd, enum hts_fmt_option opt, va_list args) {
fd->rqueue = hts_tpool_process_init(fd->pool,
p->qsize ? p->qsize : hts_tpool_size(fd->pool)*2,
0);
pthread_mutex_init(&fd->metrics_lock, NULL);
pthread_mutex_init(&fd->ref_lock, NULL);
pthread_mutex_init(&fd->range_lock, NULL);
pthread_mutex_init(&fd->bam_list_lock, NULL);
}
fd->shared_ref = 1; // Needed to avoid clobbering ref between threads
fd->own_pool = 0;
Expand Down

0 comments on commit 80ab890

Please sign in to comment.