Uninitialized variable in -[WebDefaultPolicyDelegate webView:decidePolicyForMIMEType:request:frame:decisionListener:]

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jim Correia]

-[WebDefaultPolicyDelegate webView:decidePolicyForMIMEType:request:frame:decisionListener:]

has the following code in its implementation:

    if ([[request URL] isFileURL]) {
        BOOL isDirectory;
        [[NSFileManager defaultManager] fileExistsAtPath:[[request URL] path] isDirectory:&isDirectory];
        
        if (isDirectory) {
            [listener ignore];


isDirectory is only filled in by the NSFileManager method in the case that the file exists. In all other cases we are using an uninitialized variable. Initializing the variable is sufficient to solve the problem.

See also rdar://problem/4908081

[David Kilzer (:ddkilzer)]

(In reply to comment #0)
> isDirectory is only filled in by the NSFileManager method in the case that the
> file exists. In all other cases we are using an uninitialized variable.
> Initializing the variable is sufficient to solve the problem.

Does the BOOL type not default to 'false' if not explicitly set?

[Alexey Proskuryakov]

Confirmed (automatic BOOL variables aren't automatically initialized).

Is this just a potential problem, or there are real life circumstances where the delegate gets called for a non-existent object?

[Jim Correia]

Yes, it is a real life problem in my application. (I provided more details in radar, which I realize is only readable by Apple people.)

Essentially I'm using a custom URL protocol to load web archives to avoid the problem where WebKit will load the original resource from the network or filesystem (which may no longer exist.) I'm not rewriting the URLs, only providing data, so the web policy delegate will see the original file url which doesn't point to an existing path.

[David Kilzer (:ddkilzer)]

Created attachment 12235 [details]
Patch v1

The obvious fix.

[Brady Eidson]

Comment on attachment 12235 [details]
Patch v1

surly you mean "BOOL isDirectory = NO"?
otherwise r+

[David Kilzer (:ddkilzer)]

Created attachment 12236 [details]
Patch v2 (change false to NO)

Now with correct-style points!

[David Kilzer (:ddkilzer)]

Committed revision 18612.

[Jeff Johnson]

This was not a good fix.  -[NSFileManager fileExistsAtPath:isDirectory] may or may not modify the variable isDirectory; that's a private implementation detail of NSFileManager.

What should be done is to check the return value of [[NSFileManager defaultManager] fileExistsAtPath:[[request URL] path] isDirectory:&isDirectory] before using isDirectory.

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> This was not a good fix.  -[NSFileManager fileExistsAtPath:isDirectory] may or
> may not modify the variable isDirectory; that's a private implementation detail
> of NSFileManager.
> 
> What should be done is to check the return value of [[NSFileManager
> defaultManager] fileExistsAtPath:[[request URL] path] isDirectory:&isDirectory]
> before using isDirectory.

Please open a new bug.

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> (In reply to comment #8)
> > This was not a good fix.  -[NSFileManager fileExistsAtPath:isDirectory] may or
> > may not modify the variable isDirectory; that's a private implementation detail
> > of NSFileManager.
> > 
> > What should be done is to check the return value of [[NSFileManager
> > defaultManager] fileExistsAtPath:[[request URL] path] isDirectory:&isDirectory]
> > before using isDirectory.
> 
> Please open a new bug.

Bug 16700.

http://trac.webkit.org/projects/webkit/changeset/18612

http://developer.apple.com/documentation/Cocoa/Reference/Foundation/Classes/NSFileManager_Class/Reference/Reference.html#//apple_ref/occ/instm/NSFileManager/fileExistsAtPath:isDirectory: