the structure demux_lock[i].answerlock is not initialized

[nm1]

Revision

11271

Issue Description

the structure demux_lock[i].answerlock is not initialised

When the issue occurs

in dvbapi

How the issue is reproducable

Look at the code module-dvbapi.c

It causes an undefined behavior.

[ua2Ahfieghah]

Currently the problem should be fixed in revision #11329 but the mutex initialisation is not thread save. The initialization can be done by two threads in parallel. With the following patch below applied against #11330, the initialization becomes thread save.

--- a/module-dvbapi.c
+++ b/module-dvbapi.c
@@ -1761,20 +1761,13 @@ void dvbapi_parse_cat(int32_t demux_id, uchar *buf, int32_t len)
        return;
 }

+static pthread_mutex_t lockindex = PTHREAD_MUTEX_INITIALIZER;
+
 ca_index_t dvbapi_get_descindex(int32_t demux_index, int32_t pid, int32_t stream_id)
 {
        int32_t i, j, k, fail = 1;
        ca_index_t idx = 0;
        uint32_t tmp_idx;
-
-       static pthread_mutex_t lockindex;
-       static int8_t init_mutex = 0;
-
-       if(init_mutex == 0)
-       {
-               SAFE_MUTEX_INIT(&lockindex, NULL);
-               init_mutex = 1;
-       }

        if(cfg.dvbapi_boxtype == BOXTYPE_NEUMO)
        {
@@ -1997,6 +1990,7 @@ void dvbapi_stop_descrambling(int32_t demux_id)
        dvbapi_stop_filter(demux_id, TYPE_ECM);

        memset(&demux[demux_id], 0 , sizeof(DEMUXTYPE));
+       SAFE_MUTEX_INIT(&demux[demux_id].answerlock, NULL);
        for(i = 0; i < ECM_PIDS; i++)
        {
                for(j = 0; j < MAX_STREAM_INDICES; j++)
@@ -4076,8 +4070,7 @@ int32_t dvbapi_net_init_listenfd(void)
        return listenfd;
 }

-static pthread_mutex_t event_handler_lock;
-static int8_t init_mutex = 0;
+static pthread_mutex_t event_handler_lock = PTHREAD_MUTEX_INITIALIZER;

 void event_handler(int32_t UNUSED(signal))
 {
@@ -4089,12 +4082,6 @@ void event_handler(int32_t UNUSED(signal))
        uchar mbuf[2048]; // dirty fix: larger buffer needed for CA PMT mode 6 with many parallel channels to decode
        if(dvbapi_client != cur_client()) { return; }

-       if(init_mutex == 0)
-       {
-               SAFE_MUTEX_INIT(&event_handler_lock, NULL);
-               init_mutex = 1;
-       }
-
        SAFE_MUTEX_LOCK(&event_handler_lock);

        if(cfg.dvbapi_boxtype == BOXTYPE_PC || cfg.dvbapi_boxtype == BOXTYPE_PC_NODMX || cfg.dvbapi_boxtype == BOXTYPE_SAMYGO)
@@ -5178,6 +5165,7 @@ static void *dvbapi_main_local(void *cli)
        memset(demux, 0, sizeof(struct demux_s) * MAX_DEMUX);
        for(i = 0; i < MAX_DEMUX; i++)
        {
+               SAFE_MUTEX_INIT(&demux[i].answerlock, NULL);
                for(j = 0; j < ECM_PIDS; j++)
                {
                        for(l = 0; l < MAX_STREAM_INDICES; l++)
@@ -5983,12 +5971,6 @@ void dvbapi_send_dcw(struct s_client *client, ECM_REQUEST *er)

                if(er->rc < E_NOTFOUND && cfg.dvbapi_requestmode == 1 && er->caid != 0) // FOUND
                {
-                       if(demux[i].init_mutex == 0)
-                       {
-                               SAFE_MUTEX_INIT(&demux[i].answerlock, NULL);
-                               demux[i].init_mutex = 1;
-                       }
-
                        SAFE_MUTEX_LOCK(&demux[i].answerlock); // only process one ecm answer

                        if(demux[i].ECMpids[j].checked != 4)
--- a/module-dvbapi.h
+++ b/module-dvbapi.h
@@ -209,7 +209,6 @@ typedef struct demux_s
        uint8_t old_ecmfiltercount; // previous ecm filtercount
        uint8_t old_emmfiltercount; // previous emm filtercount
        pthread_mutex_t answerlock; // requestmode 1 avoid race
-       int8_t init_mutex;
 #ifdef WITH_STAPI
        uint32_t DescramblerHandle[PTINUM];
        int32_t desc_pidcount;

[theparasol]

I'm not experienced with mutex and threadsafety but oscam uses on several places in the code mutexes.
I see your proposed changes includes "PTHREAD_MUTEX_INITIALIZER"

Scanning in whole oscam code it seems only be used in coolapi before so I suspect more parts of oscam can be improved!

Can you review if the mutex usage in the code is sound/safe:

notroot@ubuntu:~/oscam-trunk$ grep "pthread_mutex_t" *.c *.h
module-dvbapi.c: static pthread_mutex_t lockindex;
module-dvbapi.c:static pthread_mutex_t event_handler_lock;
module-dvbapi-coolapi.c: pthread_mutex_t mutex;
module-dvbapi-coolapi.c:static pthread_mutex_t demux_lock = PTHREAD_MUTEX_INITIALIZER;
module-dvbapi-coolapi-legacy.c: pthread_mutex_t mutex;
module-dvbapi-stapi5.c:static pthread_mutex_t filter_lock;
module-dvbapi-stapi.c:static pthread_mutex_t filter_lock;
module-gbox-sms.c:static pthread_mutex_t sleep_cond_mutex;
module-gbox-sms.c:static pthread_mutex_t sms_mutex;
module-ghttp.c: pthread_mutex_t conn_mutex;
module-serial.c:static pthread_mutex_t mutex;
module-webif-lib.c: pthread_mutex_t mutex;
oscam.c: static pthread_mutex_t mutex;
oscam.c: if(pthread_mutex_trylock(&mutex))
oscam.c:static pthread_mutex_t reader_check_sleep_cond_mutex;
oscam.c: pthread_mutex_t card_poll_sleep_cond_mutex;
oscam-ecm.c:static pthread_mutex_t cw_process_sleep_cond_mutex;
oscam-garbage.c:static pthread_mutex_t add_lock;
oscam-garbage.c:static pthread_mutex_t sleep_cond_mutex;
oscam-log.c:static pthread_mutex_t log_thread_sleep_cond_mutex;
oscam-log.c:static pthread_mutex_t log_mutex;
oscam-time.c:void sleepms_on_cond(const char *n, pthread_mutex_t *mutex, pthread_cond_t *cond, uint32_t msec)
oscam-time.c:void cs_pthread_cond_init(const char *n, pthread_mutex_t *mutex, pthread_cond_t *cond)
oscam-time.c:void cs_pthread_cond_init_nolog(const char *n, pthread_mutex_t *mutex, pthread_cond_t *cond)
oscam-work.c: int32_t lock_status = pthread_mutex_trylock(&cl->thread_lock);
globals.h: pthread_mutex_t lock;
globals.h: pthread_mutex_t thread_lock;
module-dvbapi.h: pthread_mutex_t answerlock; requestmode 1 avoid race
oscam-time.h:void cs_pthread_cond_init(const char *n, pthread_mutex_t *mutex, pthread_cond_t *cond);
oscam-time.h:void cs_pthread_cond_init_nolog(const char *n, pthread_mutex_t *mutex, pthread_cond_t *cond);
oscam-time.h:void sleepms_on_cond(const char *n, pthread_mutex_t *mutex, pthread_cond_t *cond, uint32_t msec);

[ua2Ahfieghah]

Yes, indeed there are many places which can be improved but we have to start somewhere and your changes got my attention as it was just changed.

Regarding the initialization of pthread_mutex_t answerlock = PTHREAD_MUTEX_INITIALIZER like this. This would work in case we use e.g. C++11 where values can be initialized like this. But I guess we want to stick to pure C and also want to support older versions of C thus this would not work. Further, the surrounding structure is reinitialized later on in the code and due to this it would not be enough. Therefore we need to (re)initialize the mutex at all places. I did the corresponding changes in both places within the supplied patch file.

Regarding the request to review. I found already some other very suspicious code. The oscam comes with an own implementation of a rwmutex. This implementation has some quirks and actually cannot and does not work reliable. To circumvent these problems the code additionally implemented some work around/hack. I alredy fixed it in my private repository (I am using git) and it looks like it is working reliable already since a couple of weeks. I am even stressing the oscam with parallel http request and it is working fine (which previously did not). I would like to keep it testing for little more time but you can expect a patch from me in the near future.

[theparasol]

http://www.streamboard.tv/oscam/changeset/11331

[Bit]

@ua2Ahfieghah: Improvements are ALWAY welcome. Please continue to contribute! Thanks in advance.

[nm1]

Why are you call function SAFE_MUTEX_INIT(&demux[i].answerlock, NULL) many times
? It causes undefined behaviour and memory leak.

[ua2Ahfieghah]

Actually I called SAFE_MUTEX_INIT at places where the structure is overwritten zeros with a call to memset. Thus the mutex is here "destroyed" through the memset which was done previously in the code and therefore the mutex needs to be (re)initialized. Further, SAFE_MUTEX_INIT is actually a pthread_mutex_init and your comment is correct that prior calling memset in dvbapi_stop_descrambling a pthread_mutex_destroy should be called to prevent memory leaks. Further, I just detected that the initialization of the event_handler_lock is still done at one place of the code. That is no longer required and wrong. To fix both issues I would consider following additional patch:

--- a/module-dvbapi.c
+++ b/module-dvbapi.c
@@ -2004,6 +2004,7 @@ void dvbapi_stop_descrambling(int32_t demux_id)
        }
        dvbapi_stop_filter(demux_id, TYPE_ECM);

+       pthread_mutex_destroy(&demux[demux_id].answerlock);
        memset(&demux[demux_id], 0 , sizeof(DEMUXTYPE));
        SAFE_MUTEX_INIT(&demux[demux_id].answerlock, NULL);
        for(i = 0; i < ECM_PIDS; i++)
@@ -5274,8 +5275,6 @@ static void *dvbapi_main_local(void *cli)
                }
        }

-       SAFE_MUTEX_INIT(&event_handler_lock, NULL);
-
        for(i = 0; i < MAX_DEMUX; i++)  // init all demuxers!
        {
                demux[i].pidindex = -1;

[theparasol]

http://www.streamboard.tv/oscam/changeset/11340