Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2377443002:
Fix another bug in AXTree caught by libfuzzer (Closed)
|
Created:
4 years, 2 months ago by dmazzoni Modified:
4 years, 2 months ago Reviewers:
aboxhall CC:
aboxhall+watch_chromium.org, chromium-reviews, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, je_julie, nektar+watch_chromium.org, yuzo+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix another bug in AXTree caught by libfuzzer
There were three cases found by libfuzzer but they all had the same
root cause. Very similar to the previous issues fixed, basically
cleaning up memory properly if we get a bogus tree with things like
circular references.
Specifically, when the root of the tree changes but then
unserialization fails, we destroy the whole tree on exiting
because there's no other way to preserve it. After deleting
the old root, we then need to clean up |node|, but not if
it was already deleted or reused.
Previously we were checking if |node| was the same as |old_root|,
but really we wanted to check if |node| was deleted at all,
since it may have been a child of |old_root|.
BUG=646795
Committed: https://crrev.com/c4d7605d5a8d55c703e6ac30754375b661478128
Cr-Commit-Position: refs/heads/master@{#421658}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address feedback #
Messages
Total messages: 23 (14 generated)
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dmazzoni@chromium.org changed reviewers: + aboxhall@chromium.org
https://codereview.chromium.org/2377443002/diff/1/ui/accessibility/ax_tree.cc File ui/accessibility/ax_tree.cc (left): https://codereview.chromium.org/2377443002/diff/1/ui/accessibility/ax_tree.cc... ui/accessibility/ax_tree.cc:251: if (node != old_root && Why is this no longer necessary? https://codereview.chromium.org/2377443002/diff/1/ui/accessibility/ax_tree.cc File ui/accessibility/ax_tree.cc (right): https://codereview.chromium.org/2377443002/diff/1/ui/accessibility/ax_tree.cc... ui/accessibility/ax_tree.cc:251: if (update_state->removed_node_ids.find(src.id) == Can you describe what this is doing in more detail in the issue description? https://codereview.chromium.org/2377443002/diff/1/ui/accessibility/ax_tree_un... File ui/accessibility/ax_tree_unittest.cc (right): https://codereview.chromium.org/2377443002/diff/1/ui/accessibility/ax_tree_un... ui/accessibility/ax_tree_unittest.cc:514: AXNodeData node3; Why node3 and node4 here? https://codereview.chromium.org/2377443002/diff/1/ui/accessibility/ax_tree_un... ui/accessibility/ax_tree_unittest.cc:523: node4.child_ids.push_back(1); Is this what causes the crash? Does it crash if node3 just points to itself without node4 present?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== Fix another bug in AXTree caught by libfuzzer There were three cases found by libfuzzer but they all had the same root cause. Very similar to the previous issues fixed, basically cleaning up memory properly if we get a bogus tree with things like circular references. BUG=646795 ========== to ========== Fix another bug in AXTree caught by libfuzzer There were three cases found by libfuzzer but they all had the same root cause. Very similar to the previous issues fixed, basically cleaning up memory properly if we get a bogus tree with things like circular references. Specifically, when the root of the tree changes but then unserialization fails, we destroy the whole tree on exiting because there's no other way to preserve it. After deleting the old root, we then need to clean up |node|, but not if it was already deleted or reused. Previously we were checking if |node| was the same as |old_root|, but really we wanted to check if |node| was deleted at all, since it may have been a child of |old_root|. BUG=646795 ==========
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2377443002/diff/1/ui/accessibility/ax_tree.cc File ui/accessibility/ax_tree.cc (left): https://codereview.chromium.org/2377443002/diff/1/ui/accessibility/ax_tree.cc... ui/accessibility/ax_tree.cc:251: if (node != old_root && On 2016/09/26 at 20:24:44, aboxhall wrote: > Why is this no longer necessary? See next comment https://codereview.chromium.org/2377443002/diff/1/ui/accessibility/ax_tree.cc File ui/accessibility/ax_tree.cc (right): https://codereview.chromium.org/2377443002/diff/1/ui/accessibility/ax_tree.cc... ui/accessibility/ax_tree.cc:251: if (update_state->removed_node_ids.find(src.id) == On 2016/09/26 at 20:24:44, aboxhall wrote: > Can you describe what this is doing in more detail in the issue description? Updated change description and added comments to code too. https://codereview.chromium.org/2377443002/diff/1/ui/accessibility/ax_tree_un... File ui/accessibility/ax_tree_unittest.cc (right): https://codereview.chromium.org/2377443002/diff/1/ui/accessibility/ax_tree_un... ui/accessibility/ax_tree_unittest.cc:514: AXNodeData node3; On 2016/09/26 at 20:24:44, aboxhall wrote: > Why node3 and node4 here? Minimized from a larger example, didn't rename. Changed to node/node2. https://codereview.chromium.org/2377443002/diff/1/ui/accessibility/ax_tree_un... ui/accessibility/ax_tree_unittest.cc:523: node4.child_ids.push_back(1); On 2016/09/26 at 20:24:44, aboxhall wrote: > Is this what causes the crash? Does it crash if node3 just points to itself without node4 present? No, that didn't crash. This was the minimal case that crashed.
lgtm https://codereview.chromium.org/2377443002/diff/1/ui/accessibility/ax_tree_un... File ui/accessibility/ax_tree_unittest.cc (right): https://codereview.chromium.org/2377443002/diff/1/ui/accessibility/ax_tree_un... ui/accessibility/ax_tree_unittest.cc:523: node4.child_ids.push_back(1); On 2016/09/27 20:19:26, dmazzoni wrote: > On 2016/09/26 at 20:24:44, aboxhall wrote: > > Is this what causes the crash? Does it crash if node3 just points to itself > without node4 present? > > No, that didn't crash. This was the minimal case > that crashed. Ohhh right, because it tries to remove it from node[3] and then again from node[4/2].
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix another bug in AXTree caught by libfuzzer There were three cases found by libfuzzer but they all had the same root cause. Very similar to the previous issues fixed, basically cleaning up memory properly if we get a bogus tree with things like circular references. Specifically, when the root of the tree changes but then unserialization fails, we destroy the whole tree on exiting because there's no other way to preserve it. After deleting the old root, we then need to clean up |node|, but not if it was already deleted or reused. Previously we were checking if |node| was the same as |old_root|, but really we wanted to check if |node| was deleted at all, since it may have been a child of |old_root|. BUG=646795 ========== to ========== Fix another bug in AXTree caught by libfuzzer There were three cases found by libfuzzer but they all had the same root cause. Very similar to the previous issues fixed, basically cleaning up memory properly if we get a bogus tree with things like circular references. Specifically, when the root of the tree changes but then unserialization fails, we destroy the whole tree on exiting because there's no other way to preserve it. After deleting the old root, we then need to clean up |node|, but not if it was already deleted or reused. Previously we were checking if |node| was the same as |old_root|, but really we wanted to check if |node| was deleted at all, since it may have been a child of |old_root|. BUG=646795 Committed: https://crrev.com/c4d7605d5a8d55c703e6ac30754375b661478128 Cr-Commit-Position: refs/heads/master@{#421658} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c4d7605d5a8d55c703e6ac30754375b661478128 Cr-Commit-Position: refs/heads/master@{#421658} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
