Bug 109449 - ASSERTION FAILED: cellObject->isTableCell(), UNKNOWN in WebCore::AccessibilityTable::cellForColumnAndRow
Summary: ASSERTION FAILED: cellObject->isTableCell(), UNKNOWN in WebCore::Accessibilit...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 107699
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-11 09:11 PST by Abhishek Arya
Modified: 2014-02-07 11:23 PST (History)
6 users (show)

See Also:


Attachments
Patch (4.06 KB, patch)
2013-02-11 23:13 PST, Dominic Mazzoni
cfleizach: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Abhishek Arya 2013-02-11 09:11:57 PST
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=164768871

Fuzzer: Inferno_twister
https://code.google.com/p/chromium/issues/detail?id=175303

Crash Type: UNKNOWN
Crash Address: 0x0000bbadbeef
Crash State:
  - crash stack -
  WebCore::AccessibilityTable::cellForColumnAndRow
  WebCore::AccessibilityTableColumn::addChildren
  WebCore::AccessibilityObject::children
  
Regressed: https://cluster-fuzz.appspot.com/revisions?range=180459:181046

Minimized Testcase (0.47 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv94Hrj46qYWS0BoIqsv_6DoqbLeEuWK1V1YEovpVEq4EoWMudGbXvniBP6wdkYX0jl0edcDXCLgqTIuiXFwG2Q4XEI_HfAUz9EsZhNwTb9LmVJ5mO9ANCafqPcn-ZSKMEvQoRl3qIUJbTcCn3yrbF8CbkXO2_HT15waWgM6BiBlsTwOR5Ws
<script>

    function buildAccessibilityTree(accessibilityObject) {
        var count = accessibilityObject.childrenCount;
        for (var i = 0; i < count; ++i)
            buildAccessibilityTree(accessibilityObject.childAtIndex(i));
    }
</script>
>><table>
    <thead>
<tbody style='rotation-code: "\"}\""; visibility: hidden; '><td><script>
    if (window.accessibilityController) {
        buildAccessibilityTree(accessibilityController.focusedElement);
    }

</script>

>
Comment 1 Dominic Mazzoni 2013-02-11 23:13:45 PST
Created attachment 187778 [details]
Patch
Comment 2 Abhishek Arya 2013-02-12 08:38:35 PST
Please note that you need to explicitly add these to security bugs for cq to work. commit-queue@webkit.org,webkit.review.bot@gmail.com. I am adding them now.
Comment 3 WebKit Review Bot 2013-02-12 08:43:32 PST
Comment on attachment 187778 [details]
Patch

Rejecting attachment 187778 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 187778, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
ILED -- saving rejects to file Source/WebCore/accessibility/AccessibilityTable.cpp.rej
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/accessibility/table-with-invisible-body-causes-crash-expected.txt
patching file LayoutTests/accessibility/table-with-invisible-body-causes-crash.html

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Chris Fleizach']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16483261
Comment 4 Abhishek Arya 2013-02-12 14:37:32 PST
I don't understand this code change. We were thinking about this as a security vuln since return static_cast<AccessibilityTableCell*>(cellObject); could cause a bad cast if object is not of type AccessibilityTableCell. Your patch is just changing an assert.
Comment 5 chris fleizach 2013-02-12 15:14:12 PST
(In reply to comment #4)
> I don't understand this code change. We were thinking about this as a security vuln since return static_cast<AccessibilityTableCell*>(cellObject); could cause a bad cast if object is not of type AccessibilityTableCell. Your patch is just changing an assert.

I think the object coming back is the right kind of class, but it returns false for isTableCell because its ignored. That might be wrong I don't have code in front of me.
Comment 6 chris fleizach 2013-02-12 15:20:31 PST
(In reply to comment #5)
> (In reply to comment #4)
> > I don't understand this code change. We were thinking about this as a security vuln since return static_cast<AccessibilityTableCell*>(cellObject); could cause a bad cast if object is not of type AccessibilityTableCell. Your patch is just changing an assert.
> 
> I think the object coming back is the right kind of class, but it returns false for isTableCell because its ignored. That might be wrong I don't have code in front of me.

Yea that looks right, but I agree we could have another check in there just to feel safer
Comment 7 Abhishek Arya 2013-02-12 15:22:15 PST
(In reply to comment #5)
> (In reply to comment #4)
> > I don't understand this code change. We were thinking about this as a security vuln since return static_cast<AccessibilityTableCell*>(cellObject); could cause a bad cast if object is not of type AccessibilityTableCell. Your patch is just changing an assert.
> 
> I think the object coming back is the right kind of class, but it returns false for isTableCell because its ignored. That might be wrong I don't have code in front of me.

We need confirmation on this. Since that determines whether it is a security bug or not :)
Comment 8 chris fleizach 2013-02-12 15:23:43 PST
(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > I don't understand this code change. We were thinking about this as a security vuln since return static_cast<AccessibilityTableCell*>(cellObject); could cause a bad cast if object is not of type AccessibilityTableCell. Your patch is just changing an assert.
> > 
> > I think the object coming back is the right kind of class, but it returns false for isTableCell because its ignored. That might be wrong I don't have code in front of me.
> 
> We need confirmation on this. Since that determines whether it is a security bug or not :)

Yea not a security bug in this specific case. the object is still the right class.
Comment 9 Abhishek Arya 2013-02-12 15:28:32 PST
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > I don't understand this code change. We were thinking about this as a security vuln since return static_cast<AccessibilityTableCell*>(cellObject); could cause a bad cast if object is not of type AccessibilityTableCell. Your patch is just changing an assert.
> > > 
> > > I think the object coming back is the right kind of class, but it returns false for isTableCell because its ignored. That might be wrong I don't have code in front of me.
> > 
> > We need confirmation on this. Since that determines whether it is a security bug or not :)
> 
> Yea not a security bug in this specific case. the object is still the right class.

Thanks a lot. fixed flags. In normal rendering land, we don't have this kind of confusion. You might want to fix more of these kind of asserts in accessibility where accessibilityIsIgnored makes sure but also not hide a bad cast bug.
Comment 10 chris fleizach 2013-02-12 15:29:04 PST
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #5)
> > > > (In reply to comment #4)
> > > > > I don't understand this code change. We were thinking about this as a security vuln since return static_cast<AccessibilityTableCell*>(cellObject); could cause a bad cast if object is not of type AccessibilityTableCell. Your patch is just changing an assert.
> > > > 
> > > > I think the object coming back is the right kind of class, but it returns false for isTableCell because its ignored. That might be wrong I don't have code in front of me.
> > > 
> > > We need confirmation on this. Since that determines whether it is a security bug or not :)
> > 
> > Yea not a security bug in this specific case. the object is still the right class.
> 
> Thanks a lot. fixed flags. In normal rendering land, we don't have this kind of confusion. You might want to fix more of these kind of asserts in accessibility where accessibilityIsIgnored makes sure but also not hide a bad cast bug.

Yea i agree, this section could be handled more cleanly
Comment 11 Radar WebKit Bug Importer 2014-02-07 11:23:11 PST
<rdar://problem/16013955>