Skip to content

Commit

Permalink
Fixed thread issues in oficonv code.
Browse files Browse the repository at this point in the history 
Fixed various thread issues in the oficonv module reported by Valgrind's
DRD tool by moving some attributes that are modified during a call to
OFiconv_open() from the structure that is shared between multiple
iconv handles (struct _citrus_iconv_shared) to the structure containing
the unique attributes of each handle (struct _citrus_iconv).
Also added a read-write lock protecting accesses to the oficonv
logger callback.

Thanks to Mathieu Malaterre <[email protected]> for the bug report
and test program demonstrating the issue.

This closes DCMTK issue #1137.
  • Loading branch information
@eichelberg
eichelberg committed Dec 29, 2024
1 parent 99dade3 commit b88017d
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 24 deletions.
6 changes: 3 additions & 3 deletions oficonv/libsrc/citrus_iconv_local.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,14 +110,14 @@ struct _citrus_iconv_shared {
_citrus_module_t ci_module;
unsigned int ci_used_count;
char *ci_convname;
bool ci_discard_ilseq;
struct iconv_hooks *ci_hooks;
bool ci_ilseq_invalid;
};

struct _citrus_iconv {
struct _citrus_iconv_shared *cv_shared;
void *cv_closure;
struct iconv_hooks *ci_hooks;
bool ci_discard_ilseq;
bool ci_ilseq_invalid;
};

#endif
12 changes: 6 additions & 6 deletions oficonv/libsrc/citrus_iconv_std.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -522,10 +522,10 @@ _citrus_iconv_std_iconv_convert(struct _citrus_iconv * cv,
tmpin = *in;
szrin = szrout = 0;
ret = mbtocsx(&sc->sc_src_encoding, &csid, &idx, &tmpin,
*inbytes, &szrin, cv->cv_shared->ci_hooks);
*inbytes, &szrin, cv->ci_hooks);

if (ret != 0 && (ret != EILSEQ ||
!cv->cv_shared->ci_discard_ilseq)) {
!cv->ci_discard_ilseq)) {
goto err;
} else if (ret == EILSEQ) {
/*
Expand Down Expand Up @@ -565,18 +565,18 @@ _citrus_iconv_std_iconv_convert(struct _citrus_iconv * cv,
* Some software depends on this behavior
* though this is against POSIX specification.
*/
if (cv->cv_shared->ci_ilseq_invalid != 0) {
if (cv->ci_ilseq_invalid != 0) {
ret = EILSEQ;
goto err;
}
inval++;
szrout = 0;
if ((((flags & _CITRUS_ICONV_F_HIDE_INVALID) == 0) &&
!cv->cv_shared->ci_discard_ilseq) &&
!cv->ci_discard_ilseq) &&
is->is_use_invalid) {
ret = wctombx(&sc->sc_dst_encoding,
*out, *outbytes, is->is_invalid,
&szrout, cv->cv_shared->ci_hooks);
&szrout, cv->ci_hooks);
if (ret)
goto err;
}
Expand All @@ -587,7 +587,7 @@ _citrus_iconv_std_iconv_convert(struct _citrus_iconv * cv,
/* csid/index -> mb */
ret = cstombx(&sc->sc_dst_encoding,
*out, *outbytes, csid, idx, &szrout,
cv->cv_shared->ci_hooks);
cv->ci_hooks);
if (ret)
goto err;
next:
Expand Down
18 changes: 12 additions & 6 deletions oficonv/libsrc/citrus_lock.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,20 @@
#ifdef HAVE_WINDOWS_H

#include <windows.h>
#define WLOCK(lock) AcquireSRWLockExclusive(lock);
#define UNLOCK(lock) ReleaseSRWLockExclusive(lock);
#define WLOCK(lock) AcquireSRWLockExclusive(lock);
#define UNLOCK(lock) ReleaseSRWLockExclusive(lock);
#define RLOCK(lock) AcquireSRWLockShared(lock);
#define UNRLOCK(lock) ReleaseSRWLockShared(lock);

#else /* HAVE_WINDOWS_H */

#ifdef HAVE_PTHREAD_H

#include <pthread.h>
#define WLOCK(lock) pthread_rwlock_wrlock(lock);
#define UNLOCK(lock) pthread_rwlock_unlock(lock);
#define WLOCK(lock) pthread_rwlock_wrlock(lock);
#define UNLOCK(lock) pthread_rwlock_unlock(lock);
#define RLOCK(lock) pthread_rwlock_rdlock(lock);
#define UNRLOCK(lock) pthread_rwlock_unlock(lock);

#else /* HAVE_PTHREAD_H */

Expand All @@ -55,8 +59,10 @@

#else /* WITH_THREADS */

#define WLOCK(lock) /* nothing */ ;
#define UNLOCK(lock) /* nothing */ ;
#define WLOCK(lock) /* nothing */ ;
#define UNLOCK(lock) /* nothing */ ;
#define RLOCK(lock) /* nothing */ ;
#define UNRLOCK(lock) /* nothing */ ;

#endif /* WITH_THREADS */

Expand Down
16 changes: 8 additions & 8 deletions oficonv/libsrc/oficonv_iconv.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,9 @@ _iconv_open(const char *out, const char *in, struct _citrus_iconv *handle)
return ((iconv_t)-1);
}

handle->cv_shared->ci_discard_ilseq = strcasestr(out, "//IGNORE");
handle->cv_shared->ci_ilseq_invalid = false;
handle->cv_shared->ci_hooks = NULL;
handle->ci_discard_ilseq = strcasestr(out, "//IGNORE");
handle->ci_ilseq_invalid = false;
handle->ci_hooks = NULL;

return ((iconv_t)(void *)handle);
}
Expand Down Expand Up @@ -342,22 +342,22 @@ OFiconvctl(iconv_t cd, int request, void *argument)
case ICONV_SET_TRANSLITERATE:
return ((*i == 1) ? 0 : -1);
case ICONV_GET_DISCARD_ILSEQ:
*i = cv->cv_shared->ci_discard_ilseq ? 1 : 0;
*i = cv->ci_discard_ilseq ? 1 : 0;
return (0);
case ICONV_SET_DISCARD_ILSEQ:
cv->cv_shared->ci_discard_ilseq = *i;
cv->ci_discard_ilseq = *i;
return (0);
case ICONV_SET_HOOKS:
cv->cv_shared->ci_hooks = hooks;
cv->ci_hooks = hooks;
return (0);
case ICONV_SET_FALLBACKS:
errno = EOPNOTSUPP;
return (-1);
case ICONV_GET_ILSEQ_INVALID:
*i = cv->cv_shared->ci_ilseq_invalid ? 1 : 0;
*i = cv->ci_ilseq_invalid ? 1 : 0;
return (0);
case ICONV_SET_ILSEQ_INVALID:
cv->cv_shared->ci_ilseq_invalid = *i;
cv->ci_ilseq_invalid = *i;
return (0);
default:
errno = EINVAL;
Expand Down
18 changes: 17 additions & 1 deletion oficonv/libsrc/oficonv_logger.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,33 @@

#include "dcmtk/config/osconfig.h"
#include "dcmtk/oficonv/iconv.h"
#include "citrus_lock.h"
#include <stdio.h>

#ifdef WITH_THREADS
#ifdef HAVE_WINDOWS_H
static SRWLOCK logger_lock = SRWLOCK_INIT;
#elif defined(HAVE_PTHREAD_H)
static pthread_rwlock_t logger_lock = PTHREAD_RWLOCK_INITIALIZER;
#endif
#endif

static oficonv_logger_callback_t logger_callback = NULL;
static int log_level = 3;

void set_oficonv_logger_callback(oficonv_logger_callback_t callback)
{
WLOCK(&logger_lock);
logger_callback = callback;
UNLOCK(&logger_lock);
}

oficonv_logger_callback_t get_oficonv_logger_callback()
{
return logger_callback;
RLOCK(&logger_lock);
oficonv_logger_callback_t result = logger_callback;
UNRLOCK(&logger_lock);
return result;
}

void set_oficonv_log_level(int level)
Expand All @@ -43,6 +57,7 @@ void set_oficonv_log_level(int level)

void oficonv_log(int level, const char *text1, const char *text2, const char *text3)
{
RLOCK(&logger_lock);
if (logger_callback) logger_callback(level, text1, text2, text3);
else
{
Expand Down Expand Up @@ -70,4 +85,5 @@ void oficonv_log(int level, const char *text1, const char *text2, const char *te
}
if (level >= log_level) fprintf(stderr, "%s %s%s%s\n", level_text, text1, text2, text3);
}
UNRLOCK(&logger_lock);
}

0 comments on commit b88017d

Please sign in to comment.