Bug 109449 – ASSERTION FAILED: cellObject->isTableCell(), UNKNOWN in WebCore::AccessibilityTable::cellForColumnAndRow

• Status: NEW
• Component: Accessibility

[Abhishek Arya]

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>

>

[Dominic Mazzoni]

Created attachment 187778 [details]
Patch

[Abhishek Arya]

Please note that you need to explicitly add these to security bugs for cq to work. [e-mail],[e-mail]. I am adding them now.

[WebKit Review Bot]

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

[Abhishek Arya]

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.

[chris fleizach]

(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.

[chris fleizach]

(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

[Abhishek Arya]

(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 :)

[chris fleizach]

(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.

[Abhishek Arya]

(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.

[chris fleizach]

(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

[Radar WebKit Bug Importer]

<rdar://problem/16013955>