diff mbox series

[FFmpeg-devel,8/9] avformat/udp: Fix temporary buffer race

Message ID 20240608231046.3619551-8-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/9] avformat/rtpenc_vc2hq: Check sizes | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Michael Niedermayer June 8, 2024, 11:10 p.m. UTC
Fixes: CID1551679 Data race condition
Fixes: CID1551687 Data race condition

Sponsored-by: Sovereign Tech Fund
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/udp.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Marton Balint July 8, 2024, 5:46 p.m. UTC | #1
On Sun, 9 Jun 2024, Michael Niedermayer wrote:

> Fixes: CID1551679 Data race condition
> Fixes: CID1551687 Data race condition

How is this a data race? Concurrent reading and writing is not supported 
for UDP as far as I know.

Thanks,
Marton

>
> Sponsored-by: Sovereign Tech Fund
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
> libavformat/udp.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/libavformat/udp.c b/libavformat/udp.c
> index c1ebdd12220..fd4847eda71 100644
> --- a/libavformat/udp.c
> +++ b/libavformat/udp.c
> @@ -107,7 +107,8 @@ typedef struct UDPContext {
>     pthread_cond_t cond;
>     int thread_started;
> #endif
> -    uint8_t tmp[UDP_MAX_PKT_SIZE+4];
> +    uint8_t tmp_rx[UDP_MAX_PKT_SIZE+4];
> +    uint8_t tmp_tx[UDP_MAX_PKT_SIZE+4];
>     int remaining_in_dg;
>     char *localaddr;
>     int timeout;
> @@ -504,7 +505,7 @@ static void *circular_buffer_task_rx( void *_URLContext)
>            see "General Information" / "Thread Cancelation Overview"
>            in Single Unix. */
>         pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, &old_cancelstate);
> -        len = recvfrom(s->udp_fd, s->tmp+4, sizeof(s->tmp)-4, 0, (struct sockaddr *)&addr, &addr_len);
> +        len = recvfrom(s->udp_fd, s->tmp_rx+4, sizeof(s->tmp_rx)-4, 0, (struct sockaddr *)&addr, &addr_len);
>         pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &old_cancelstate);
>         pthread_mutex_lock(&s->mutex);
>         if (len < 0) {
> @@ -516,7 +517,7 @@ static void *circular_buffer_task_rx( void *_URLContext)
>         }
>         if (ff_ip_check_source_lists(&addr, &s->filters))
>             continue;
> -        AV_WL32(s->tmp, len);
> +        AV_WL32(s->tmp_rx, len);
>
>         if (av_fifo_can_write(s->fifo) < len + 4) {
>             /* No Space left */
> @@ -532,7 +533,7 @@ static void *circular_buffer_task_rx( void *_URLContext)
>                 goto end;
>             }
>         }
> -        av_fifo_write(s->fifo, s->tmp, len + 4);
> +        av_fifo_write(s->fifo, s->tmp_rx, len + 4);
>         pthread_cond_signal(&s->cond);
>     }
>
> @@ -581,9 +582,9 @@ static void *circular_buffer_task_tx( void *_URLContext)
>         len = AV_RL32(tmp);
>
>         av_assert0(len >= 0);
> -        av_assert0(len <= sizeof(s->tmp));
> +        av_assert0(len <= sizeof(s->tmp_tx));
>
> -        av_fifo_read(s->fifo, s->tmp, len);
> +        av_fifo_read(s->fifo, s->tmp_tx, len);
>
>         pthread_mutex_unlock(&s->mutex);
>
> @@ -607,7 +608,7 @@ static void *circular_buffer_task_tx( void *_URLContext)
>             target_timestamp = start_timestamp + sent_bits * 1000000 / s->bitrate;
>         }
>
> -        p = s->tmp;
> +        p = s->tmp_tx;
>         while (len) {
>             int ret;
>             av_assert0(len > 0);
> -- 
> 2.45.2
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Michael Niedermayer July 9, 2024, 1:12 p.m. UTC | #2
On Mon, Jul 08, 2024 at 07:46:19PM +0200, Marton Balint wrote:
> 
> 
> On Sun, 9 Jun 2024, Michael Niedermayer wrote:
> 
> > Fixes: CID1551679 Data race condition
> > Fixes: CID1551687 Data race condition
> 
> How is this a data race? Concurrent reading and writing is not supported for
> UDP as far as I know.

maybe coverity tricked me together with memory of a long standing
unreproduceable data corruption bug in udp ... thinking there where
2 threads using the same temporary buffer (which would really fit
the bug i remembered)

feel free to revert!

thx

[...]
Michael Niedermayer July 9, 2024, 1:18 p.m. UTC | #3
On Tue, Jul 09, 2024 at 03:12:52PM +0200, Michael Niedermayer wrote:
> On Mon, Jul 08, 2024 at 07:46:19PM +0200, Marton Balint wrote:
> > 
> > 
> > On Sun, 9 Jun 2024, Michael Niedermayer wrote:
> > 
> > > Fixes: CID1551679 Data race condition
> > > Fixes: CID1551687 Data race condition

marked the 2 as false positives


> > 
> > How is this a data race? Concurrent reading and writing is not supported for
> > UDP as far as I know.
> 
> maybe coverity tricked me together with memory of a long standing
> unreproduceable data corruption bug in udp ... thinking there where
> 2 threads using the same temporary buffer (which would really fit
> the bug i remembered)
> 
> feel free to revert!

[...]
diff mbox series

Patch

diff --git a/libavformat/udp.c b/libavformat/udp.c
index c1ebdd12220..fd4847eda71 100644
--- a/libavformat/udp.c
+++ b/libavformat/udp.c
@@ -107,7 +107,8 @@  typedef struct UDPContext {
     pthread_cond_t cond;
     int thread_started;
 #endif
-    uint8_t tmp[UDP_MAX_PKT_SIZE+4];
+    uint8_t tmp_rx[UDP_MAX_PKT_SIZE+4];
+    uint8_t tmp_tx[UDP_MAX_PKT_SIZE+4];
     int remaining_in_dg;
     char *localaddr;
     int timeout;
@@ -504,7 +505,7 @@  static void *circular_buffer_task_rx( void *_URLContext)
            see "General Information" / "Thread Cancelation Overview"
            in Single Unix. */
         pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, &old_cancelstate);
-        len = recvfrom(s->udp_fd, s->tmp+4, sizeof(s->tmp)-4, 0, (struct sockaddr *)&addr, &addr_len);
+        len = recvfrom(s->udp_fd, s->tmp_rx+4, sizeof(s->tmp_rx)-4, 0, (struct sockaddr *)&addr, &addr_len);
         pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &old_cancelstate);
         pthread_mutex_lock(&s->mutex);
         if (len < 0) {
@@ -516,7 +517,7 @@  static void *circular_buffer_task_rx( void *_URLContext)
         }
         if (ff_ip_check_source_lists(&addr, &s->filters))
             continue;
-        AV_WL32(s->tmp, len);
+        AV_WL32(s->tmp_rx, len);
 
         if (av_fifo_can_write(s->fifo) < len + 4) {
             /* No Space left */
@@ -532,7 +533,7 @@  static void *circular_buffer_task_rx( void *_URLContext)
                 goto end;
             }
         }
-        av_fifo_write(s->fifo, s->tmp, len + 4);
+        av_fifo_write(s->fifo, s->tmp_rx, len + 4);
         pthread_cond_signal(&s->cond);
     }
 
@@ -581,9 +582,9 @@  static void *circular_buffer_task_tx( void *_URLContext)
         len = AV_RL32(tmp);
 
         av_assert0(len >= 0);
-        av_assert0(len <= sizeof(s->tmp));
+        av_assert0(len <= sizeof(s->tmp_tx));
 
-        av_fifo_read(s->fifo, s->tmp, len);
+        av_fifo_read(s->fifo, s->tmp_tx, len);
 
         pthread_mutex_unlock(&s->mutex);
 
@@ -607,7 +608,7 @@  static void *circular_buffer_task_tx( void *_URLContext)
             target_timestamp = start_timestamp + sent_bits * 1000000 / s->bitrate;
         }
 
-        p = s->tmp;
+        p = s->tmp_tx;
         while (len) {
             int ret;
             av_assert0(len > 0);