1413087 - stylo: Stack overflow [@ AllocationSize] near servo/components/style/rule_tree/mod.rs:826

Status: RESOLVED WONTFIX

(Core :: CSS Parsing and Computation, defect, P4)

Description

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Testcase found while fuzzing mozilla-central rev a89e5587c7a7.

==2401==ERROR: AddressSanitizer: stack-overflow on address 0x7ffce591ffd8 (pc 0x00000041e50f bp 0x7ffce5920870 sp 0x7ffce591ffe0 T0)
    #0 0x41e50e in AllocationSize /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_allocator.cc:617:20
    #1 0x41e50e in __asan::asan_malloc_usable_size(void const*, unsigned long, unsigned long) /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_allocator.cc:775
    #2 0x7f4fbff5fd75 in malloc_size_of::{{impl}}::malloc_size_of<style::rule_tree::RuleNode> /builds/worker/workspace/build/src/servo/components/malloc_size_of/lib.rs:122
    #3 0x7f4fbff5fd75 in _$LT$style..rule_tree..RuleNode$u20$as$u20$malloc_size_of..MallocSizeOf$GT$::size_of::heafa65466fbaf5c2 /builds/worker/workspace/build/src/servo/components/style/rule_tree/mod.rs:826
    #4 0x7f4fbff5fd86 in _$LT$style..rule_tree..RuleNode$u20$as$u20$malloc_size_of..MallocSizeOf$GT$::size_of::heafa65466fbaf5c2 /builds/worker/workspace/build/src/servo/components/style/rule_tree/mod.rs:827
    #5 0x7f4fbff5fd86 in _$LT$style..rule_tree..RuleNode$u20$as$u20$malloc_size_of..MallocSizeOf$GT$::size_of::heafa65466fbaf5c2 /builds/worker/workspace/build/src/servo/components/style/rule_tree/mod.rs:827
    #6 0x7f4fbff5fd86 in _$LT$style..rule_tree..RuleNode$u20$as$u20$malloc_size_of..MallocSizeOf$GT$::size_of::heafa65466fbaf5c2 /builds/worker/workspace/build/src/servo/components/style/rule_tree/mod.rs:827
    #7 0x7f4fbff5fd86 in _$LT$style..rule_tree..RuleNode$u20$as$u20$malloc_size_of..MallocSizeOf$GT$::size_of::heafa65466fbaf5c2 /builds/worker/workspace/build/src/servo/components/style/rule_tree/mod.rs:827
    #8 0x7f4fbff5fd86 in _$LT$style..rule_tree..RuleNode$u20$as$u20$malloc_size_of..MallocSizeOf$GT$::size_of::heafa65466fbaf5c2 /builds/worker/workspace/build/src/servo/components/style/rule_tree/mod.rs:827
    #9 0x7f4fbff5fd86 in _$LT$style..rule_tree..RuleNode$u20$as$u20$malloc_size_of..MallocSizeOf$GT$::size_of::heafa65466fbaf5c2 /builds/worker/workspace/build/src/servo/components/style/rule_tree/mod.rs:827
    #10 0x7f4fbff5fd86 in _$LT$style..rule_tree..RuleNode$u20$as$u20$malloc_size_of..MallocSizeOf$GT$::size_of::heafa65466fbaf5c2 /builds/worker/workspace/build/src/servo/components/style/rule_tree/mod.rs:827
    #11 0x7f4fbff5fd86 in _$LT$style..rule_tree..RuleNode$u20$as$u20$malloc_size_of..MallocSizeOf$GT$::size_of::heafa65466fbaf5c2 /builds/worker/workspace/build/src/servo/components/style/rule_tree/mod.rs:827
    #12 0x7f4fbff5fd86 in _$LT$style..rule_tree..RuleNode$u20$as$u20$malloc_size_of..MallocSizeOf$GT$::size_of::heafa65466fbaf5c2 /builds/worker/workspace/build/src/servo/components/style/rule_tree/mod.rs:827
    #13 0x7f4fbff5fd86 in _$LT$style..rule_tree..RuleNode$u20$as$u20$malloc_size_of..MallocSizeOf$GT$::size_of::heafa65466fbaf5c2 /builds/worker/workspace/build/src/servo/components/style/rule_tree/mod.rs:827
    #14 0x7f4fbff5fd86 in _$LT$style..rule_tree..RuleNode$u20$as$u20$malloc_size_of..MallocSizeOf$GT$::size_of::heafa65466fbaf5c2 /builds/worker/workspace/build/src/servo/components/style/rule_tree/mod.rs:827
    #15 0x7f4fbff5fd86 in _$LT$style..rule_tree..RuleNode$u20$as$u20$malloc_size_of..MallocSizeOf$GT$::size_of::heafa65466fbaf5c2 /builds/worker/workspace/build/src/servo/components/style/rule_tree/mod.rs:827
    #16 0x7f4fbff5fd86 in _$LT$style..rule_tree..RuleNode$u20$as$u20$malloc_size_of..MallocSizeOf$GT$::size_of::heafa65466fbaf5c2 /builds/worker/workspace/build/src/servo/components/style/rule_tree/mod.rs:827
    #17 0x7f4fbff5fd86 in _$LT$style..rule_tree..RuleNode$u20$as$u20$malloc_size_of..MallocSizeOf$GT$::size_of::heafa65466fbaf5c2 /builds/worker/workspace/build/src/servo/components/style/rule_tree/mod.rs:827
    #18 0x7f4fbff5fd86 in _$LT$style..rule_tree..RuleNode$u20$as$u20$malloc_size_of..MallocSizeOf$GT$::size_of::heafa65466fbaf5c2 /builds/worker/workspace/build/src/servo/components/style/rule_tree/mod.rs:827
    #19 0x7f4fbff5fd86 in _$LT$style..rule_tree..RuleNode$u20$as$u20$malloc_size_of..MallocSizeOf$GT$::size_of::heafa65466fbaf5c2 /builds/worker/workspace/build/src/servo/components/style/rule_tree/mod.rs:827
    #20 0x7f4fbff5fd86 in _$LT$style..rule_tree..RuleNode$u20$as$u20$malloc_size_of..MallocSizeOf$GT$::size_of::heafa65466fbaf5c2 /builds/worker/workspace/build/src/servo/components/style/rule_tree/mod.rs:827

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: next.html

Comment 2

[Jason Kratzer [:jkratzer]]

This testcase requires the fuzzpriv extension which can be found at the following URL:
https://github.com/MozillaSecurity/domfuzz/tree/master/dom/extension

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Stack overflow in MallocSizeOf. njn, could you take a look?

I'm wondering whether we are having some cycle reference in the rule tree somehow...

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Looks like legit stack exhaustion though. To repro you can just open next.html and measure memory.

That page is creating a huge amount of rules matching an element, so the tree is very deep.

Comment 5

[Chris Peterson [:cpeterson]]

status-firefox57=wontfix unless someone thinks this bug should block 57

Comment 6

[Ryan VanderMeulen [:RyanVM]]

INFO: Last good revision: 71d4452dd938d6522e13e7c97bd0298d04852d77
INFO: First bad revision: 6b9d06ba6f769234530ae67d8353377d58a93fd0
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=71d4452dd938d6522e13e7c97bd0298d04852d77&tochange=6b9d06ba6f769234530ae67d8353377d58a93fd0

Comment 7

[Nicholas Nethercote [inactive]]

Here's the function in question.

> #[cfg(feature = "gecko")]
> impl MallocSizeOf for RuleNode {
>     fn size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
>         let mut n = 0;
>         for child in self.iter_children() {
>             n += unsafe { ops.malloc_size_of(child.ptr()) };
>             n += unsafe { (*child.ptr()).size_of(ops) };
>         }
>         n
>     }
> }

It's a straightforward tree traversal. There's a single local variable (n) so the size of the stack frame shouldn't be excessive. I guess I could convert it to use an explicit stack, which could be a Vec and so would be resizable and wouldn't suffer from the size limitations of the system stack.

Emilio, do you think that's worthwhile? Or could this test case be considered pathological?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

I don't think it's worth it...

Comment 9

[Chris Peterson [:cpeterson]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> I don't think it's worth it...

Should we just close this bug as WONTFIX? Will fuzzing be blocked if we don't fix this stack overflow?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

I guess that's a question for Jason.

Comment 11

[Jason Kratzer [:jkratzer]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> I guess that's a question for Jason.

I think that'll be fine.  This issue has been triggered 40 times in the last 5 days which isn't very much.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

WONTFIX per comment 11. If it happens to block stuff we can fix, thanks Jason!

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> It's a straightforward tree traversal. There's a single local variable (n)
> so the size of the stack frame shouldn't be excessive. I guess I could
> convert it to use an explicit stack, which could be a Vec and so would be
> resizable and wouldn't suffer from the size limitations of the system stack.

I don't think we need an explicit stack... We can just use a deque to record all nodes we need to traverse to, i.e. a BFS approach, which shouldn't be much harder than DFS approach like how it is currently done here...