1477329 - ASan: WasmTextToBinary reinterpret a Vector<wasm::AstName> to a Vector<wasm::AstValType>.

Status: RESOLVED FIXED

(Core :: Javascript: WebAssembly, defect)

Description

[Nicolas B. Pierron [:nbp]]

This issue can be reproduced with the patch attached to Bug 1467116 with ASan and either debug/optimized build, and by running the test case wasm/gc/ref.js with --wasm-gc

==10734==ERROR: AddressSanitizer: use-after-poison on address 0x6210003aef30 at pc 0x0000033a6ddf bp 0x7ffd94a916c0 sp 0x7ffd94a916b0

#14 0x00000000033a6ddf in js::wasm::AstValType::code (this=0x6210003aef30) at /home/nicolas/mozilla/wksp-6/js/src/wasm/WasmAST.h:161
#15 0x00000000033a6979 in js::wasm::AstFuncType::hash (ft=...) at /home/nicolas/mozilla/wksp-6/js/src/wasm/WasmAST.h:376
#16 0x00000000033a5be5 in js::detail::HashTable<js::HashMapEntry<js::wasm::AstFuncType*, unsigned int>, js::HashMap<js::wasm::AstFuncType*, unsigned int, js::wasm::AstFuncType, js::LifoAllocPolicy<(js::Fallibility)0> >::MapHashPolicy, js::LifoAllocPolicy<(js::Fallibility)0> >::prepareHash (l=...)
    at /home/nicolas/mozilla/_build/js/bugzil.la/1470115/wip/x64/clang/dbg/dist/include/js/HashTable.h:1284
#17 0x00000000033a58b0 in js::detail::HashTable<js::HashMapEntry<js::wasm::AstFuncType*, unsigned int>, js::HashMap<js::wasm::AstFuncType*, unsigned int, js::wasm::AstFuncType, js::LifoAllocPolicy<(js::Fallibility)0> >::MapHashPolicy, js::LifoAllocPolicy<(js::Fallibility)0> >::lookupForAdd (this=0x6210003ae958, l=...)
    at /home/nicolas/mozilla/_build/js/bugzil.la/1470115/wip/x64/clang/dbg/dist/include/js/HashTable.h:1839
#18 0x00000000033a3e74 in js::HashMap<js::wasm::AstFuncType*, unsigned int, js::wasm::AstFuncType, js::LifoAllocPolicy<(js::Fallibility)0> >::lookupForAdd (this=0x6210003ae958,
    l=...) at /home/nicolas/mozilla/_build/js/bugzil.la/1470115/wip/x64/clang/dbg/dist/include/js/HashTable.h:152
#19 0x0000000003393927 in js::wasm::AstModule::append (this=0x6210003ae920, funcType=0x6210003aefa8) at /home/nicolas/mozilla/wksp-6/js/src/wasm/WasmAST.h:1322
#20 0x0000000003321c96 in ParseModule (
    text=0x6210003bed00 u"(module\n      (type $cons (struct\n", ' ' <repeats 19 times>, "(field $car i32)\n", ' ' <repeats 19 times>, "(field $cdr (ref $cons))))\n\n      (type $odd (struct\n", ' ' <repeats 18 times>, "(field $x i32)\n", ' ' <repeats 18 times>, "(field "..., stackLimit=140727095535361, lifo=..., error=0x7ffd94a92410,
    binary=0x7ffd94a92050) at /home/nicolas/mozilla/wksp-6/js/src/wasm/WasmTextToBinary.cpp:4049
#21 0x00000000033214c2 in js::wasm::TextToBinary (
    text=0x6210003bed00 u"(module\n      (type $cons (struct\n", ' ' <repeats 19 times>, "(field $car i32)\n", ' ' <repeats 19 times>, "(field $cdr (ref $cons))))\n\n      (type $odd (struct\n", ' ' <repeats 18 times>, "(field $x i32)\n", ' ' <repeats 18 times>, "(field "..., stackLimit=140727095535361, bytes=0x7ffd94a923c0, offsets=0x7ffd94a92430,
    error=0x7ffd94a92410) at /home/nicolas/mozilla/wksp-6/js/src/wasm/WasmTextToBinary.cpp:5937
#22 0x0000000001c80bad in WasmTextToBinary (cx=0x7ffd94a921a0, argc=1, vp=0x6210003a5d98) at /home/nicolas/mozilla/wksp-6/js/src/builtin/TestingFunctions.cpp:676

(rr) frame 15
(rr) ptype ft.args_
type = class mozilla::Vector<js::wasm::AstValType, 0, js::LifoAllocPolicy<js::Fallibility::Fallible> >


Setting a breakpoint in the function which returns the allocated vector returns:

Thread 1 hit Breakpoint 1, js::detail::BumpChunk::tryAlloc (this=0x6210003ae900, n=32) at /home/nicolas/mozilla/wksp-6/js/src/ds/LifoAlloc.h:460
460             uint8_t* newBump = nextAllocEnd(aligned, n);
(rr) bt
#0  js::detail::BumpChunk::tryAlloc (this=0x6210003ae900, n=32) at /home/nicolas/mozilla/wksp-6/js/src/ds/LifoAlloc.h:460
#1  0x0000000000ad2d4d in js::LifoAlloc::allocImpl (this=0x7ffd94a91fe0, n=32) at /home/nicolas/mozilla/wksp-6/js/src/ds/LifoAlloc.h:603
#2  0x0000000000ad2be2 in js::LifoAlloc::alloc (this=0x7ffd94a91fe0, n=32) at /home/nicolas/mozilla/wksp-6/js/src/ds/LifoAlloc.h:676
#3  0x000000000339a697 in js::LifoAllocPolicy<(js::Fallibility)0>::maybe_pod_malloc<js::wasm::AstName> (this=0x7ffd94a91300, numElems=2)
    at /home/nicolas/mozilla/wksp-6/js/src/ds/LifoAlloc.h:1028
#4  0x000000000339a25d in js::LifoAllocPolicy<(js::Fallibility)0>::pod_malloc<js::wasm::AstName> (this=0x7ffd94a91300, numElems=2)
    at /home/nicolas/mozilla/wksp-6/js/src/ds/LifoAlloc.h:1050
#5  0x000000000339a098 in mozilla::detail::VectorImpl<js::wasm::AstName, 0ul, js::LifoAllocPolicy<(js::Fallibility)0>, false>::growTo (aV=..., aNewCap=2)
    at /home/nicolas/mozilla/_build/js/bugzil.la/1470115/wip/x64/clang/dbg/dist/include/mozilla/Vector.h:137
#6  0x00000000033999be in mozilla::Vector<js::wasm::AstName, 0ul, js::LifoAllocPolicy<(js::Fallibility)0> >::growStorageBy (this=0x7ffd94a91300, aIncr=1)
    at /home/nicolas/mozilla/_build/js/bugzil.la/1470115/wip/x64/clang/dbg/dist/include/mozilla/Vector.h:1054
#7  0x00000000033a02a5 in mozilla::Vector<js::wasm::AstName, 0ul, js::LifoAllocPolicy<(js::Fallibility)0> >::append<js::wasm::AstName&> (this=0x7ffd94a91300, aU=...)
    at /home/nicolas/mozilla/_build/js/bugzil.la/1470115/wip/x64/clang/dbg/dist/include/mozilla/Vector.h:1409
#8  0x000000000336023a in ParseStructFields (c=..., st=0x7ffd94a91830) at /home/nicolas/mozilla/wksp-6/js/src/wasm/WasmTextToBinary.cpp:3426
#9  0x0000000003343124 in ParseTypeDef (c=...) at /home/nicolas/mozilla/wksp-6/js/src/wasm/WasmTextToBinary.cpp:3455
#10 0x0000000003321c51 in ParseModule (
    text=0x6210003bed00 u"(module\n      (type $cons (struct\n", ' ' <repeats 19 times>, "(field $car i32)\n", ' ' <repeats 19 times>, "(field $cdr (ref $cons))))\n\n      (type $odd (struct\n", ' ' <repeats 18 times>, "(field $x i32)\n", ' ' <repeats 18 times>, "(field "..., stackLimit=140727095535361, lifo=..., error=0x7ffd94a92410,
    binary=0x7ffd94a92050) at /home/nicolas/mozilla/wksp-6/js/src/wasm/WasmTextToBinary.cpp:4046
#11 0x00000000033214c2 in js::wasm::TextToBinary (
    text=0x6210003bed00 u"(module\n      (type $cons (struct\n", ' ' <repeats 19 times>, "(field $car i32)\n", ' ' <repeats 19 times>, "(field $cdr (ref $cons))))\n\n      (type $odd (struct\n", ' ' <repeats 18 times>, "(field $x i32)\n", ' ' <repeats 18 times>, "(field "..., stackLimit=140727095535361, bytes=0x7ffd94a923c0, offsets=0x7ffd94a92430,
    error=0x7ffd94a92410) at /home/nicolas/mozilla/wksp-6/js/src/wasm/WasmTextToBinary.cpp:5937
#12 0x0000000001c80bad in WasmTextToBinary (cx=0x7ffd94a921a0, argc=1, vp=0x6210003a5d98) at /home/nicolas/mozilla/wksp-6/js/src/builtin/TestingFunctions.cpp:676

Somehow, the vector got allocated as Vector<js::wasm::AstName> and is being used as Vector<js::wasm::AstValType> which has a different size for its elements.

Comment 1

[Nicolas B. Pierron [:nbp]]

The problem is that ParseTypeDef creates a new type with:

    type = new(c.lifo) AstStructType(name, std::move(st));

returns it to ParseModule (case WasmToken::Type), and always do a static cast into an AstFuncType.

Comment 2

[Lars T Hansen [:lth]]

I will fix.

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1477329-no-static-cast.patch

The problem here really was that we allowed static_cast to be used in the first place, when we already have checked type transfer functions that should be used.  Probably a consequence of evolving the code over time.

I just removed static_cast everywhere and introduced a helper for append(), which is somewhat redundant since it only has one caller, but it's clean.

Comment 4

[Lars T Hansen [:lth]]

Looks like this will need to be uplifted to beta.

Comment 5

[Julian Seward [:jseward]]

Comment on attachment 8994418 [details] [diff] [review]
bug1477329-no-static-cast.patch

LGTM, + restores V-cleanness in the test case from dup bug 1477913.

Comment 6

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a095dcb31ff53d06ed8ea61e592b6477fcb97cb

Comment 7

[Lars T Hansen [:lth]]

Actually, a beta uplift is not necessary.  The bug is in the text-to-binary functionality which is only in the JS shell, not in the browser.

Daniel, can you open this bug?  It is not a security concern.

Comment 9

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0a095dcb31ff