[GStreamer] clang analysis: Unlocked access in ImageDecoderGStreamer.cpp

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alicia Boya García]

In ImageDecoderGStreamer.cpp, line 384:

        if (!decoder.m_messageDispatched) {
            Locker locker { decoder.m_messageLock };
            decoder.m_messageCondition.wait(decoder.m_messageLock);
        }

m_messageDispatched is guarded by m_messageLock (see ImageDecoderGStreamer.h):

    bool m_messageDispatched WTF_GUARDED_BY_LOCK(m_messageLock) { false };

Yet the value is being checked before taking the lock. I don't know how it should work but it sure looks like it shouldn't be that way given those guard preconditions.

[Philippe Normand]

Created attachment 431335 [details]
Patch

[Alicia Boya García]

Comment on attachment 431335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431335&action=review

> Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.cpp:380
> +                if (!decoder.m_messageDispatched)

LGTM. Normally you use while() instead of if(), but it should be fine if the condition is only signalled when m_messageDispatched is set to true, rather than to any value.

[Philippe Normand]

(In reply to Alicia Boya García from comment #2)
> Comment on attachment 431335 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431335&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.cpp:380
> > +                if (!decoder.m_messageDispatched)
> 
> LGTM. Normally you use while() instead of if(), but it should be fine if the
> condition is only signalled when m_messageDispatched is set to true, rather
> than to any value.

Right, I think there's no need for a while() indeed. The condition is notified at most one time.

[EWS]

Committed r278925 (238856@main): <https://commits.webkit.org/238856@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431335 [details].