Bug 154364 – [Cocoa] Always check the return value of dlopen() and dlsym() in Release builds

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Kilzer (:ddkilzer)]

Always check the return value of dlopen() and dlsym() in Release builds.

This would have caught a soft linking error sooner for an internal issue I was debugging.

[David Kilzer (:ddkilzer)]

Created attachment 271601 [details]
Patch v1

[Alexey Proskuryakov]

Comment on attachment 271601 [details]
Patch v1

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

> Source/WebCore/platform/mac/SoftLinking.h:37
> +        static void* dylib = ^{ \

What is the purpose of adding the blocks?

[Alexey Proskuryakov]

Comment on attachment 271601 [details]
Patch v1

Oops, that's actually pretty clear without explanation.

[Andy Estes]

Comment on attachment 271601 [details]
Patch v1

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

> Source/WebCore/platform/mac/SoftLinking.h:324
>          ASSERT_WITH_MESSAGE_UNUSED(isOptional, isOptional || frameworkLibrary, "%s", dlerror()); \

Do we still need this assert?

[WebKit Commit Bot]

Comment on attachment 271601 [details]
Patch v1

Clearing flags on attachment: 271601

Committed r196744: <http://trac.webkit.org/changeset/196744>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

Comment on attachment 271601 [details]
Patch v1

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

>> Source/WebCore/platform/mac/SoftLinking.h:37
>> +        static void* dylib = ^{ \
> 
> What is the purpose of adding the blocks?

I saw you answered your own question, but for posterity, this is so we can use a RELEASE_ASSERT_WITH_MESSAGE that only fires once instead of each time the method is called.

>> Source/WebCore/platform/mac/SoftLinking.h:324
>>          ASSERT_WITH_MESSAGE_UNUSED(isOptional, isOptional || frameworkLibrary, "%s", dlerror()); \
> 
> Do we still need this assert?

Oops!  No, we can remove this.  Will fix in a followup.  Good catch!

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> Comment on attachment 271601 [details]
> Patch v1
> 
> Clearing flags on attachment: 271601
> 
> Committed r196744: <http://trac.webkit.org/changeset/196744>

Follow-up fix for Andy's comment:

Committed r196763: < http://trac.webkit.org/changeset/196763>

[Andy Estes]

A couple more questions:

1. Should we also RELEASE_ASSERT that objc_getClass() doesn't return nil in SOFT_LINK_CLASS? Seems like that case would also cause some mysterious crashes (e.g. uncaught NSInvalidArgumentExceptions) and bugs.

2. Shouldn't we use dispatch_once/std::call_once to initialize these static locals? Since soft-linked symbols are lazy-loaded, and we often use macros that look like the real symbol names, it would be easy to accidentally introduce a race condition. For instance:

    #import "QuickLookSoftLink.h"

    NSURL *url = ...;
    NSURLSession *session = [NSURLSession sharedSession];
    [session dataTaskWithURL:url completionHandler:^(NSData *, NSURLResponse *response, NSError *) {
        // This is called on the session's delegate queue
        if (response.URL.scheme isEqualToString:QLPreviewScheme])
            ...
    }];

    NSSet *mimeTypes = QLPreviewGetSupportedMIMETypes(); // main thread

QLPreviewScheme and QLPreviewGetSupportedMIMETypes() look like a normal pointer value and function name, but they're really macros that expand to functions that will ultimately both call QuickLookLibrary() if the framework hasn't already been loaded.

[David Kilzer (:ddkilzer)]

This change caused:

Bug 154703: REGRESSION (r196744): NetworkExtension.framework and NEFilterSource class are not available on Recovery partition
<https://bugs.webkit.org/show_bug.cgi?id=154703>

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> A couple more questions:
> 
> 1. Should we also RELEASE_ASSERT that objc_getClass() doesn't return nil in
> SOFT_LINK_CLASS? Seems like that case would also cause some mysterious
> crashes (e.g. uncaught NSInvalidArgumentExceptions) and bugs.

Committed r197149: <http://trac.webkit.org/changeset/197149>

> 2. Shouldn't we use dispatch_once/std::call_once to initialize these static
> locals? Since soft-linked symbols are lazy-loaded, and we often use macros
> that look like the real symbol names, it would be easy to accidentally
> introduce a race condition. For instance:
> 
>     #import "QuickLookSoftLink.h"
> 
>     NSURL *url = ...;
>     NSURLSession *session = [NSURLSession sharedSession];
>     [session dataTaskWithURL:url completionHandler:^(NSData *, NSURLResponse
> *response, NSError *) {
>         // This is called on the session's delegate queue
>         if (response.URL.scheme isEqualToString:QLPreviewScheme])
>             ...
>     }];
> 
>     NSSet *mimeTypes = QLPreviewGetSupportedMIMETypes(); // main thread
> 
> QLPreviewScheme and QLPreviewGetSupportedMIMETypes() look like a normal
> pointer value and function name, but they're really macros that expand to
> functions that will ultimately both call QuickLookLibrary() if the framework
> hasn't already been loaded.

QLPreviewScheme and QLPreviewGetSupportedMIMETypes are initialized using dispatch_once in SOFT_LINK_POINTER_FOR_SOURCE() and SOFT_LINK_FUNCTION_FOR_SOURCE() macros.

Are you suggesting the non *_FOR_SOURCE() versions of the macros need dispatch_once versions?  That would be SOFT_LINK_POINTER() and SOFT_LINK(), as well as:

SOFT_LINK_MAY_FAIL()
SOFT_LINK_OPTIONAL()
SOFT_LINK_CLASS()
SOFT_LINK_CLASS_OPTIONAL()
SOFT_LINK_POINTER_OPTIONAL()
SOFT_LINK_CONSTANT()
SOFT_LINK_CONSTANT_MAY_FAIL()

[Andy Estes]

(In reply to comment #11)
> (In reply to comment #9)
> > A couple more questions:
> > 
> > 1. Should we also RELEASE_ASSERT that objc_getClass() doesn't return nil in
> > SOFT_LINK_CLASS? Seems like that case would also cause some mysterious
> > crashes (e.g. uncaught NSInvalidArgumentExceptions) and bugs.
> 
> Committed r197149: <http://trac.webkit.org/changeset/197149>

Thanks!

> 
> > 2. Shouldn't we use dispatch_once/std::call_once to initialize these static
> > locals? Since soft-linked symbols are lazy-loaded, and we often use macros
> > that look like the real symbol names, it would be easy to accidentally
> > introduce a race condition. For instance:
> > 
> >     #import "QuickLookSoftLink.h"
> > 
> >     NSURL *url = ...;
> >     NSURLSession *session = [NSURLSession sharedSession];
> >     [session dataTaskWithURL:url completionHandler:^(NSData *, NSURLResponse
> > *response, NSError *) {
> >         // This is called on the session's delegate queue
> >         if (response.URL.scheme isEqualToString:QLPreviewScheme])
> >             ...
> >     }];
> > 
> >     NSSet *mimeTypes = QLPreviewGetSupportedMIMETypes(); // main thread
> > 
> > QLPreviewScheme and QLPreviewGetSupportedMIMETypes() look like a normal
> > pointer value and function name, but they're really macros that expand to
> > functions that will ultimately both call QuickLookLibrary() if the framework
> > hasn't already been loaded.
> 
> QLPreviewScheme and QLPreviewGetSupportedMIMETypes are initialized using
> dispatch_once in SOFT_LINK_POINTER_FOR_SOURCE() and
> SOFT_LINK_FUNCTION_FOR_SOURCE() macros.

I guess I chose a bad example, then :)

> 
> Are you suggesting the non *_FOR_SOURCE() versions of the macros need
> dispatch_once versions?  That would be SOFT_LINK_POINTER() and SOFT_LINK(),
> as well as:
> 
> SOFT_LINK_MAY_FAIL()
> SOFT_LINK_OPTIONAL()
> SOFT_LINK_CLASS()
> SOFT_LINK_CLASS_OPTIONAL()
> SOFT_LINK_POINTER_OPTIONAL()
> SOFT_LINK_CONSTANT()
> SOFT_LINK_CONSTANT_MAY_FAIL()

Yes, the init##...() functions in these macros need dispatch_once to protect the setting of the static function pointer.

Also, SOFT_LINK_LIBRARY() and all variants of SOFT_LINK_FRAMEWORK() need to use dispatch_once when initializing their static void*s.