Opened 7 years ago
Last modified 7 years ago
#4486 new defect
the structure demux_lock[i].answerlock is not initialized
Reported by: | nm1 | Owned by: | |
---|---|---|---|
Priority: | major | Component: | DVBApi |
Severity: | low | Keywords: | |
Cc: | Sensitive: | no |
Description
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.
Change History (9)
comment:1 by , 7 years ago
Summary: | the structure demux_lock[i].answerlock is not initialised → the structure demux_lock[i].answerlock is not initialized |
---|
comment:2 by , 7 years ago
comment:3 by , 7 years ago
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);
comment:4 by , 7 years ago
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.
comment:6 by , 7 years ago
@ua2Ahfieghah: Improvements are ALWAY welcome. Please continue to contribute! Thanks in advance.
comment:7 by , 7 years ago
Why are you call function SAFE_MUTEX_INIT(&demux[i].answerlock, NULL) many times
? It causes undefined behaviour and memory leak.
comment:8 by , 7 years ago
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;
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.