Bug 120300 - AX:Null pointer may be dereferenced.
Summary: AX:Null pointer may be dereferenced.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Lukasz Gajowy
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-08-26 05:25 PDT by Lukasz Gajowy
Modified: 2013-08-28 12:40 PDT (History)
10 users (show)

See Also:


Attachments
Patch (1.75 KB, patch)
2013-08-27 06:18 PDT, Lukasz Gajowy
cfleizach: review-
l.gajowy: commit-queue?
Details | Formatted Diff | Diff
Patch #2 (1.82 KB, patch)
2013-08-27 23:31 PDT, Lukasz Gajowy
cfleizach: review-
l.gajowy: commit-queue?
Details | Formatted Diff | Diff
Patch #3 (1.85 KB, patch)
2013-08-27 23:44 PDT, Lukasz Gajowy
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lukasz Gajowy 2013-08-26 05:25:52 PDT
In function getOrCreate(Widget *widget) from AXObjectCache.cpp: null pointer 'newObj' that comes from line 325 may be dereferenced in method getAXID() and below this method.
Comment 1 Radar WebKit Bug Importer 2013-08-26 05:26:24 PDT
<rdar://problem/14833466>
Comment 2 chris fleizach 2013-08-26 12:22:40 PDT
that would indicate that we're passing in a new Widget type. 
Certainly that should be protected and ASSERTED, but I don't think that is actually happening anywhere
Comment 3 Lukasz Gajowy 2013-08-27 06:18:11 PDT
Created attachment 209761 [details]
Patch
Comment 4 chris fleizach 2013-08-27 08:57:30 PDT
Comment on attachment 209761 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:334
> +    // Will crash later when widget type is other than FrameView or ScrollBar.

This comment should read something like -- "Catch the case if an (unsupported) widget type is used. Only FrameView and ScrollBar are supported now."

> Source/WebCore/accessibility/AXObjectCache.cpp:335
> +    ASSERT(newObj);

we should also do a if (!newObject) return; here
the ASSERT will catch this problem in DEBUG, but not in release
Comment 5 Lukasz Gajowy 2013-08-27 23:31:21 PDT
Created attachment 209849 [details]
Patch #2
Comment 6 chris fleizach 2013-08-27 23:34:03 PDT
Comment on attachment 209849 [details]
Patch #2

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

> Source/WebCore/accessibility/AXObjectCache.cpp:334
> +    // Catch the case if an (unsupported) widget type is used. Only FrameView and ScrollBar are supported now.

But we should still have the ASSERT. We want both assert and (!newObj) check. 
That way if it happens in DEBUG we'll know right away, but if it happens in release, no one will crash

Thanks
Comment 7 Lukasz Gajowy 2013-08-27 23:44:27 PDT
Created attachment 209850 [details]
Patch #3

Sorry, I misread your review the last time. No it is the way you explained it.
Comment 8 chris fleizach 2013-08-27 23:45:34 PDT
Comment on attachment 209850 [details]
Patch #3

thanks. looks good. let's make sure the EWS bots are good before committing
Comment 9 Build Bot 2013-08-28 00:23:46 PDT
Comment on attachment 209850 [details]
Patch #3

Attachment 209850 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1605174
Comment 10 chris fleizach 2013-08-28 12:15:13 PDT
Comment on attachment 209850 [details]
Patch #3

Windows build bot seems busted, so moving forward with this
Comment 11 WebKit Commit Bot 2013-08-28 12:40:10 PDT
Comment on attachment 209850 [details]
Patch #3

Clearing flags on attachment: 209850

Committed r154767: <http://trac.webkit.org/changeset/154767>
Comment 12 WebKit Commit Bot 2013-08-28 12:40:12 PDT
All reviewed patches have been landed.  Closing bug.