Bug 226495 - [GStreamer] clang analysis: Unlocked access in ImageDecoderGStreamer.cpp
Summary: [GStreamer] clang analysis: Unlocked access in ImageDecoderGStreamer.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-06-01 04:57 PDT by Alicia Boya García
Modified: 2021-06-16 01:16 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.50 KB, patch)
2021-06-14 09:26 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alicia Boya García 2021-06-01 04:57:15 PDT
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.
Comment 1 Philippe Normand 2021-06-14 09:26:29 PDT
Created attachment 431335 [details]
Patch
Comment 2 Alicia Boya García 2021-06-15 06:47:17 PDT
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.
Comment 3 Philippe Normand 2021-06-15 10:27:16 PDT
(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.
Comment 4 EWS 2021-06-16 01:16:00 PDT
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].