[FFmpeg-devel] [PATCH] fftools: silence coverity race condition warning (PR #20638)

[Niklas Haas via ffmpeg-devel <ffmpeg-devel@ffmpeg.org>]

PR #20638 opened by Niklas Haas (haasn)
URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20638
Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20638.patch

Avoids the following Coverity warning:

> CID 1666425:         Concurrent data access violations  (MISSING_LOCK)
> Accessing "fg->best_input" without holding lock "Scheduler.schedule_lock".
> Elsewhere, "SchFilterGraph.best_input" is written to with
> "Scheduler.schedule_lock" held 2 out of 2 times (2 of these accesses
> strongly imply that it is necessary).

I'm pretty sure this is a false positive, as `fg->best_input` is only ever
written by the filter thread - so there's zero risk of concurrent write with
this read. However, coverity seems to not be aware of this fact. To be honest,
I'm not sure why it didn't trigger before, but possibly because the only
other writing location before was in the same function, so maybe it was
smart enough to understand that it can't race with itself.

A mutex is nowehere near expensive enough that this once-per-frame extra
lock/unlock is going to be in any way measurable.


>From 8248d22720e36edf1fc04269e047c5218526f14d Mon Sep 17 00:00:00 2001
From: Niklas Haas <git@haasn.dev>
Date: Wed, 1 Oct 2025 18:15:48 +0200
Subject: [PATCH] fftools: silence coverity race condition warning

Avoids the following Coverity warning:

> CID 1666425:         Concurrent data access violations  (MISSING_LOCK)
> Accessing "fg->best_input" without holding lock "Scheduler.schedule_lock".
> Elsewhere, "SchFilterGraph.best_input" is written to with
> "Scheduler.schedule_lock" held 2 out of 2 times (2 of these accesses
> strongly imply that it is necessary).

I'm pretty sure this is a false positive, as `fg->best_input` is only ever
written by the filter thread - so there's zero risk of concurrent write with
this read. However, coverity seems to not be aware of this fact. To be honest,
I'm not sure why it didn't trigger before, but possibly because the only
other writing location before was in the same function, so maybe it was
smart enough to understand that it can't race with itself.

A mutex is nowehere near expensive enough that this once-per-frame extra
lock/unlock is going to be in any way measurable.
---
 fftools/ffmpeg_sched.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/fftools/ffmpeg_sched.c b/fftools/ffmpeg_sched.c
index d08f4a061d..5c52779588 100644
--- a/fftools/ffmpeg_sched.c
+++ b/fftools/ffmpeg_sched.c
@@ -2445,19 +2445,16 @@ int sch_filter_receive(Scheduler *sch, unsigned fg_idx,
 
     av_assert0(*in_idx <= fg->nb_inputs);
 
-    // update scheduling to account for desired input stream, if it changed
-    //
-    // this check needs no locking because only the filtering thread
-    // updates this value
-    if (*in_idx != fg->best_input) {
-        pthread_mutex_lock(&sch->schedule_lock);
+    pthread_mutex_lock(&sch->schedule_lock);
 
+    // update scheduling to account for desired input stream, if it changed
+    if (*in_idx != fg->best_input) {
         fg->best_input = *in_idx;
         schedule_update_locked(sch);
-
-        pthread_mutex_unlock(&sch->schedule_lock);
     }
 
+    pthread_mutex_unlock(&sch->schedule_lock);
+
     if (*in_idx == fg->nb_inputs) {
         int terminate = waiter_wait(sch, &fg->waiter);
         return terminate ? AVERROR_EOF : AVERROR(EAGAIN);
-- 
2.49.1

_______________________________________________
ffmpeg-devel mailing list -- ffmpeg-devel@ffmpeg.org
To unsubscribe send an email to ffmpeg-devel-leave@ffmpeg.org