555102 - NPE in Traits::_buildTraitsBindings

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Description

[Lars T Hansen]

Attachment: Fuzzed input

Argo Release 3868:5f716dbd8596 with some local patches; -Dinterp.

Fuzzed input as attached (based on hello2.abc).

Crashes in above mentioned function with this==0, with stack as follows:

#0  0x0003acd6 in avmplus::Namespace::isPublic (this=0x0) at /Users/lhansen/work/tamarin-argo/platform/mac/avmshell/../../../core/Namespace.cpp:87
#1  0x0003b49c in avmplus::NamespaceSet::create (gc=0x5a8000, ns=0x0) at /Users/lhansen/work/tamarin-argo/platform/mac/avmshell/../../../core/NamespaceSet.cpp:55
#2  0x0004becd in avmplus::Traits::_buildTraitsBindings (this=0x55aa80, toplevel=0x55e040, abcGen=0xbfffec10) at /Users/lhansen/work/tamarin-argo/platform/mac/avmshell/../../../core/Traits.cpp:1210
#3  0x0004ec93 in avmplus::Traits::resolveSignaturesSelf (this=0x55aa80, toplevel=0x55e040) at /Users/lhansen/work/tamarin-argo/platform/mac/avmshell/../../../core/Traits.cpp:1453
#4  0x0004f328 in ~List [inlined] () at /Users/lhansen/work/tamarin-argo/core/avmplusList.h:1421
#5  0x0004f328 in avmplus::Traits::resolveSignatures (this=0x55aa80, toplevel=0x55e040) at /Users/lhansen/work/tamarin-argo/platform/mac/avmshell/../../../core/Traits.cpp:1421
#6  0x0004f7c2 in avmplus::Traits::newCatchTraits (toplevel=0x0, pool=0x522c28, traitsPos=0x5c9269 "", name=0x5b06d0, ns=0x0) at /Users/lhansen/work/tamarin-argo/platform/mac/avmshell/../../../core/Traits.cpp:517
#7  0x0005739e in avmplus::Verifier::parseExceptionHandlers (this=0xbfffefac) at /Users/lhansen/work/tamarin-argo/platform/mac/avmshell/../../../core/Verifier.cpp:2846
#8  0x000630ac in avmplus::Verifier::verify (this=0xbfffefac, emitter=0xbfffeffc) at /Users/lhansen/work/tamarin-argo/platform/mac/avmshell/../../../core/Verifier.cpp:323
#9  0x00039215 in MMgc::GCWeakRef::get () at /Users/lhansen/work/tamarin-argo/MMgc/GCWeakRef.h:402
#10 0x00039215 in avmplus::MethodInfo::getMethodSignature () at /Users/lhansen/work/tamarin-argo/core/MethodInfo-inlines.h:403
#11 0x00039215 in avmplus::MethodInfo::setInterpImpl () at /Users/lhansen/work/tamarin-argo/core/MethodInfo.cpp:146
#12 0x00039215 in avmplus::MethodInfo::verify (this=0x588d30, toplevel=0x55e040, abc_env=0x588dc0) at /Users/lhansen/work/tamarin-argo/platform/mac/avmshell/../../../core/MethodInfo.cpp:377
#13 0x00039598 in avmplus::MethodInfo::verifyCoerceEnter (env=0x5c5700, argc=0, args=0xbffff0c0) at /Users/lhansen/work/tamarin-argo/platform/mac/avmshell/../../../core/MethodInfo.cpp:238
#14 0x0010ebbe in avmplus::callprop_b<avmplus::Toplevel*> (env=0x55e040, base=5775457, multiname=0x5c33b8, argc=0, atomv=0xbffff0c0, vtable=0x569ce8, b=0x19) at MethodEnv-inlines.h:166
#15 0x0002c9a2 in avmplus::interpBoxed (env=0x5c5670, _argc=0, _atomv=0xbffff308) at Toplevel-inlines.h:47

Comment 1

[Edwin Smith]

may be related to bug 548684

Comment 2

[Krzysztof Palacz]

no crashes in Marlin shell

Comment 3

[Krzysztof Palacz]

In debug and debugger builds the reported error is identical to error in bug 555052 (and the stack trace is different).

Comment 4

[Krzysztof Palacz]

Attachment: stronger checks for types to be matched by exception handlers

Variable naming suggests that the exception type filter is a QName, abcFormat doesn't say much about the precise type, but the VM code can't handle wildcard namespaces. This patch adds extra range checking for exception type filters and wildcard namespaces.

Comment 5

[Edwin Smith]

Comment on attachment 436607 [details] [diff] [review]
stronger checks for types to be matched by exception handlers

Lets roll the check into resolveQName, with resolveQName calling m.isBinding().

Comment 6

[Lars T Hansen]

(In reply to comment #5)
> (From update of attachment 436607 [details] [diff] [review])
> Lets roll the check into resolveQName, with resolveQName calling m.isBinding().

BTW, if you discover any new constraints on ABC code, or any clarifications on existing constraints, please make sure to annotate the avm2overview.doc /and send mail to me and Leslie Lewis that you have done so/.  I've handed the doc over to Leslie for conversion to Livedocs, which means any updates we do locally will not be seen unless we track them.

Comment 7

[Krzysztof Palacz]

So I thought about rolling this check into resolveQName() but if this method were to contain m.isBinding() or m.isAnyName(), that would preclude wildcard names, which would actually make at least some sense in exception type filters. I can't tell if that is, will be or could actually be legal.

Comment 8

[Edwin Smith]

catch (T e) { } was never meant to work with wildcards for T; T should just be a compile-time known reference to a type name, same as for other contexts (slot/method types, class base types, etc).  Might be a neat feature but it would be a new one.

(spec bug, probably)

Comment 9

[Edwin Smith]

Comment on attachment 436607 [details] [diff] [review]
stronger checks for types to be matched by exception handlers

Jeff, chime in if you have any input on the use of wildcards in this context.

Comment 10

[Jeff Dyer]

Currently, wildcard qualifiers can only bind to e4x names. 

Wildcard qualifiers on type annotations would be interesting (catch any value whose type has a simple name of T), but probably not very easy to type check. You could have a different typed value for each invocation of the catch.

Comment 11

[Krzysztof Palacz]

Attachment: checks the name of the exception variable.

I originally misread the code, the issue is with the name of the exception variable, not its type.  Better checks of the exception type would still be needed.

Comment 12

[Edwin Smith]

Comment on attachment 437470 [details] [diff] [review]
checks the name of the exception variable.

Now just need to check for backwards compatibility with coral and declassify if possible.

Comment 13

[Jeff Dyer]

Comment on attachment 437470 [details] [diff] [review]
checks the name of the exception variable.

The check for 'isPublic' over constrains the name. ASC probably emit a public name in this binding context, but other compilers may not. All that matters with regard to verification is that a valid binding name is emitted. Without the isPublic check, it's a +r=jodyer

Comment 14

[Krzysztof Palacz]

None of the Marlin shells crash, the debug builds report failed asserts and continue though (this would qualify for declassification).
If we want to reproduce Marlin behavior exactly, then instead of what the patch does, we could ignore the  namespace part completely (and pass, say,  OBJECT_TYPE->ns() to newCatchTraits()). Opinions?

Comment 15

[Edwin Smith]

okay, sounds like:

1. we can declassify, good
2. marlin quietly accepts names that are !isBinding(), and presumably if there is a reference to the name in the catch block, it may or may not bind to the catch variable name, and will do whatever is appropriate.   (if the reference in code used the same exact multiname as the name definition, matches may still work...)
3. if we remove the isPublic clause in the patch as Jeff suggests, Argo will throw a verify failure for such non isBinding() names.

If that's right, I'm willing to make a judgement call that its okay for Argo to throw the verify error.  We could back that decision up with some this-never-happens-in-brightspot data.

I'd be leery of plucking the namespace from some other object, just seems too weird, since there is already a way for a QName to be given.

Comment 16

[Edwin Smith]

Comment on attachment 437470 [details] [diff] [review]
checks the name of the exception variable.

(I guess + means feedback given...)

Comment 17

[Krzysztof Palacz]

No additional verify failures with the isBinding() check in Brightspot.

Comment 18

[Dan Smith]

Declassifying.

Comment 19

[Krzysztof Palacz]

pushed to tr-argo
http://asteam.macromedia.com/hg/tamarin-redux-argo/rev/c4d251f3abc3

Comment 20

[Krzysztof Palacz]

pushed to tr
http://hg.mozilla.org/tamarin-redux/rev/c4d251f3abc3

Comment 21

[Chris Peyer]

Pushed regression testcase (attachment 435051 [details]) to redux changeset 4431 c7c681ec3c78