{"action":"created","issue":{"url":"https://api.github.com/repos/raspberrypi/userland/issues/228","repository_url":"https://api.github.com/repos/raspberrypi/userland","labels_url":"https://api.github.com/repos/raspberrypi/userland/issues/228/labels{/name}","comments_url":"https://api.github.com/repos/raspberrypi/userland/issues/228/comments","events_url":"https://api.github.com/repos/raspberrypi/userland/issues/228/events","html_url":"https://github.com/raspberrypi/userland/issues/228","id":62105525,"number":228,"title":"Potential thread safety issues with MMAL","user":{"login":"wm4","id":1387750,"avatar_url":"https://avatars.githubusercontent.com/u/1387750?v=3","gravatar_id":"","url":"https://api.github.com/users/wm4","html_url":"https://github.com/wm4","followers_url":"https://api.github.com/users/wm4/followers","following_url":"https://api.github.com/users/wm4/following{/other_user}","gists_url":"https://api.github.com/users/wm4/gists{/gist_id}","starred_url":"https://api.github.com/users/wm4/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/wm4/subscriptions","organizations_url":"https://api.github.com/users/wm4/orgs","repos_url":"https://api.github.com/users/wm4/repos","events_url":"https://api.github.com/users/wm4/events{/privacy}","received_events_url":"https://api.github.com/users/wm4/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":10,"created_at":"2015-03-16T14:56:35Z","updated_at":"2016-12-30T21:06:06Z","closed_at":null,"body":"I believe I found an issue in mmal_queue. Here's its implementation:\n\n``` C\nMMAL_BUFFER_HEADER_T *mmal_queue_get(MMAL_QUEUE_T *queue)\n{\n   MMAL_BUFFER_HEADER_T *buffer;\n\n    if(!queue) return 0;\n\n   vcos_mutex_lock(&queue->lock);\n   buffer = queue->first;\n   if(!buffer)\n   {\n      vcos_mutex_unlock(&queue->lock);\n      return 0;\n   }\n\n   /* coverity[lock] This semaphore isn't being used as a mutex */\n   vcos_semaphore_wait(&queue->semaphore); /* Will always succeed */\n\n   queue->first = buffer->next;\n   if(!queue->first) queue->last = &queue->first;\n\n   queue->length--;\n   vcos_mutex_unlock(&queue->lock);\n\n   return buffer;\n}\n\nMMAL_BUFFER_HEADER_T *mmal_queue_wait(MMAL_QUEUE_T *queue)\n{\n    if(!queue) return 0;\n\n    vcos_semaphore_wait(&queue->semaphore);\n   vcos_semaphore_post(&queue->semaphore);\n   return mmal_queue_get(queue);\n}\n```\n\nWhat happens if 2 threads calls `mmal_queue_wait()` at about the same time? Thread 1 could be preempted right after calling `vcos_semaphore_post()`, during which thread 2 successfully runs the same code and dequeues a buffer. Now assume the queue size was 1, and thread 2 just reduced it to 0. Thread 1 already verified that the queue size was 1 (which is now outdated), and would return NULL instead of waiting. (This could be easily avoided by using a condition variable instead of a semaphore. Or the cheap and dumb solution: loop if the result is NULL.)\n\nI found another potential issue with buffers. This is how the `mmal_buffer_header_release()` is implemented:\n\n``` C\nvoid mmal_buffer_header_release(MMAL_BUFFER_HEADER_T *header)\n{\n#ifdef ENABLE_MMAL_EXTRA_LOGGING\n   LOG_TRACE(\"%p (%i)\", header, (int)header->priv->refcount-1);\n#endif\n\n   if(--header->priv->refcount != 0)\n      return;\n\n  [...]\n```\n\nAs you can see, there is no lock around changing and checking `refcount`. This makes the function not thread-safe. Yet, `example_basic_1.c` uses it in a context where threading is used:\n\n``` C\nstatic void input_callback(MMAL_PORT_T *port, MMAL_BUFFER_HEADER_T *buffer)\n{\n   struct CONTEXT_T *ctx = (struct CONTEXT_T *)port->userdata;\n\n   /* The decoder is done with the data, just recycle the buffer header into its pool */\n   mmal_buffer_header_release(buffer);\n\n   /* Kick the processing thread */\n   vcos_semaphore_post(&ctx->semaphore);\n}\n```\n\nNow, maybe this is actually fine, because you could argue that the buffer is owned only by 1 thread, and referencing a buffer in another thread is not allowed. So the example happens to use this in a safe way. But then, why have refcounting?\n\nIs this a bug? If not, could thread-safety be explicitly documented?\n\n(Just by the way, I'm trying to implement video decoding with the mmal API. Although the API is pretty simple, I'm still struggling to make it stable. For example, it tends to randomly freeze the whole rpi depending on exact timing, and inserting random `usleep(1000)` in my code \"fixes\" it to some degree. This happens only with some files, while other files work perfectly fine. omxplayer never has problems. I'm still trying to find out why.)\n"},"comment":{"url":"https://api.github.com/repos/raspberrypi/userland/issues/comments/269821885","html_url":"https://github.com/raspberrypi/userland/issues/228#issuecomment-269821885","issue_url":"https://api.github.com/repos/raspberrypi/userland/issues/228","id":269821885,"user":{"login":"popcornmix","id":1053730,"avatar_url":"https://avatars.githubusercontent.com/u/1053730?v=3","gravatar_id":"","url":"https://api.github.com/users/popcornmix","html_url":"https://github.com/popcornmix","followers_url":"https://api.github.com/users/popcornmix/followers","following_url":"https://api.github.com/users/popcornmix/following{/other_user}","gists_url":"https://api.github.com/users/popcornmix/gists{/gist_id}","starred_url":"https://api.github.com/users/popcornmix/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/popcornmix/subscriptions","organizations_url":"https://api.github.com/users/popcornmix/orgs","repos_url":"https://api.github.com/users/popcornmix/repos","events_url":"https://api.github.com/users/popcornmix/events{/privacy}","received_events_url":"https://api.github.com/users/popcornmix/received_events","type":"User","site_admin":false},"created_at":"2016-12-30T21:06:06Z","updated_at":"2016-12-30T21:06:06Z","body":"I think https://github.com/raspberrypi/userland/commit/ab0b1c1839d931d2a1a10da6ef5ca255de57e3d3 may fix the thread safety issue with two receive threads."}}
{"actor":{"display_login":"popcornmix"}}