[FFmpeg-devel] [PATCH] libavformat/UDP Protocol : seg fault on closing

[Laurent BRULET]

Le 14/01/2012 00:11, Michael Niedermayer a écrit :
> On Fri, Jan 13, 2012 at 09:44:17PM +0100, Laurent BRULET wrote:
>> Le 13/01/2012 20:28, Michael Niedermayer a écrit :
>>> On Fri, Jan 13, 2012 at 07:15:46PM +0100, Laurent BRULET wrote:
> [...]
>>>
>>>> circular_buffer_task, h)) { av_log(h, AV_LOG_ERROR, "pthread_create
>>>> failed\n"); goto fail; @@ -617,12 +619,18 @@ static int
>>>> udp_write(URLContext *h, const uint8_t *buf, int size) static int
>>>> udp_close(URLContext *h) { UDPContext *s = h->priv_data; + int
>>>> ret;
>>>>
>>>> if (s->is_multicast && (h->flags & AVIO_FLAG_READ))
>>>> udp_leave_multicast_group(s->udp_fd, (struct sockaddr
>>>> *)&s->dest_addr); closesocket(s->udp_fd); av_fifo_free(s->fifo);
>>>> #if HAVE_PTHREADS + s->exit_thread = 1;
>>>> + ret = pthread_join(s->circular_buffer_thread, NULL);
>>> Is this safe in case the thread has not been started at all?
>> I think it is safe because, if the thread cannot start, then udp_open
>> returns with an error. And in ffurl_connect, in case of error
>> URLContext.is_connected is not set to 1. However, in ffurl_close,
>> udp_close is only called if URLContext.is_connect equals to 1.
>> I will send a new patch (a not linewrapped one, I hope)
> the thread also wont be started/used if circular_buffer_size=0
Right, and there is also a condition on AVIO_FLAG_READ. Therefore, I add a thread_started flag in UDPContext which is set to 1 if the thread has started.
I also added error checking on pthread_mutex_init and pthread_cond_init (udp_open).

--
Laurent

---
diff --git a/libavformat/udp.c b/libavformat/udp.c
index cdcd136..9694ad2 100644
--- a/libavformat/udp.c
+++ b/libavformat/udp.c
@@ -71,6 +71,8 @@ typedef struct {
     pthread_t circular_buffer_thread;
     pthread_mutex_t mutex;
     pthread_cond_t cond;
+    int thread_started;
+    volatile int exit_thread;
 #endif
     uint8_t tmp[UDP_MAX_PKT_SIZE+4];
     int remaining_in_dg;
@@ -327,7 +329,7 @@ static void *circular_buffer_task( void *_URLContext)
     fd_set rfds;
     struct timeval tv;
 
-    for(;;) {
+    while(!s->exit_thread) {
         int left;
         int ret;
         int len;
@@ -525,18 +527,36 @@ static int udp_open(URLContext *h, const char *uri, int flags)
 
 #if HAVE_PTHREADS
     if (!is_output && s->circular_buffer_size) {
+        int ret;
+
         /* start the task going */
         s->fifo = av_fifo_alloc(s->circular_buffer_size);
-        pthread_mutex_init(&s->mutex, NULL);
-        pthread_cond_init(&s->cond, NULL);
-        if (pthread_create(&s->circular_buffer_thread, NULL, circular_buffer_task, h)) {
-            av_log(h, AV_LOG_ERROR, "pthread_create failed\n");
+        ret = pthread_mutex_init(&s->mutex, NULL);
+        if (ret != 0) {
+            av_log(h, AV_LOG_ERROR, "pthread_mutex_init failed : %s\n", strerror(ret));
             goto fail;
         }
+        ret = pthread_cond_init(&s->cond, NULL);
+        if (ret != 0) {
+            av_log(h, AV_LOG_ERROR, "pthread_cond_init failed : %s\n", strerror(ret));
+            goto cond_fail;
+        }
+        ret = pthread_create(&s->circular_buffer_thread, NULL, circular_buffer_task, h);
+        if (ret != 0) {
+            av_log(h, AV_LOG_ERROR, "pthread_create failed : %s\n", strerror(ret));
+            goto thread_fail;
+        }
+        s->thread_started = 1;
     }
 #endif
 
     return 0;
+#if HAVE_PTHREADS
+ thread_fail:
+    pthread_cond_destroy(&s->cond);
+ cond_fail:
+    pthread_mutex_destroy(&s->mutex);
+#endif
  fail:
     if (udp_fd >= 0)
         closesocket(udp_fd);
@@ -617,12 +637,20 @@ static int udp_write(URLContext *h, const uint8_t *buf, int size)
 static int udp_close(URLContext *h)
 {
     UDPContext *s = h->priv_data;
+    int ret;
 
     if (s->is_multicast && (h->flags & AVIO_FLAG_READ))
         udp_leave_multicast_group(s->udp_fd, (struct sockaddr *)&s->dest_addr);
     closesocket(s->udp_fd);
     av_fifo_free(s->fifo);
 #if HAVE_PTHREADS
+    if (s->thread_started) {
+        s->exit_thread = 1;
+        ret = pthread_join(s->circular_buffer_thread, NULL);
+        if (ret != 0)
+            av_log(h, AV_LOG_ERROR, "pthread_join(): %s\n", strerror(ret));
+    }
+
     pthread_mutex_destroy(&s->mutex);
     pthread_cond_destroy(&s->cond);
 #endif