If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

NPE in Traits::_buildTraitsBindings

VERIFIED FIXED in flash10.1

Status

Tamarin
Virtual Machine
P2
normal
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: Lars T Hansen, Assigned: Krzysztof Palacz)

Tracking

(Blocks: 1 bug)

unspecified
flash10.1
Dependency tree / graph
Bug Flags:
in-testsuite +
flashplayer-injection +
flashplayer-qrb +

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
Created attachment 435051 [details]
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

Updated

8 years ago
Assignee: nobody → kpalacz
Status: NEW → ASSIGNED
Flags: flashplayer-qrb+
Priority: -- → P2
Target Milestone: --- → flash10.1

Comment 1

8 years ago
may be related to bug 548684
(Assignee)

Comment 2

8 years ago
no crashes in Marlin shell
(Assignee)

Comment 3

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

Updated

8 years ago
Depends on: 555052
(Assignee)

Comment 4

8 years ago
Created attachment 436607 [details] [diff] [review]
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.
Attachment #436607 - Flags: review?(edwsmith)

Comment 5

8 years ago
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().
Attachment #436607 - Flags: review?(edwsmith) → review-
(Reporter)

Comment 6

8 years ago
(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.
(Assignee)

Comment 7

8 years ago
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

8 years ago
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)

Updated

8 years ago
Attachment #436607 - Flags: feedback?(jodyer)

Comment 9

8 years ago
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

8 years ago
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.

Updated

8 years ago
Attachment #436607 - Flags: feedback?(jodyer) → feedback+

Updated

8 years ago
Flags: flashplayer-injection+
(Assignee)

Comment 11

8 years ago
Created attachment 437470 [details] [diff] [review]
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.
Attachment #436607 - Attachment is obsolete: true
Attachment #437470 - Flags: superreview?(edwsmith)
Attachment #437470 - Flags: review?(jodyer)

Comment 12

8 years ago
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.
Attachment #437470 - Flags: superreview?(edwsmith) → superreview+

Comment 13

8 years ago
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
Attachment #437470 - Flags: review?(jodyer) → review-
(Assignee)

Comment 14

8 years ago
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?
(Assignee)

Updated

8 years ago
Attachment #437470 - Flags: feedback?(edwsmith)

Comment 15

8 years ago
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

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

(I guess + means feedback given...)
Attachment #437470 - Flags: feedback?(edwsmith) → feedback+

Updated

8 years ago
Flags: in-testsuite?
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 17

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

Comment 18

8 years ago
Declassifying.
Group: tamarin-security
(Assignee)

Comment 19

8 years ago
pushed to tr-argo
http://asteam.macromedia.com/hg/tamarin-redux-argo/rev/c4d251f3abc3
(Assignee)

Comment 20

8 years ago
pushed to tr
http://hg.mozilla.org/tamarin-redux/rev/c4d251f3abc3
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Attachment #437470 - Flags: review- → review+

Comment 21

8 years ago
Pushed regression testcase (attachment 435051 [details]) to redux changeset 4431 c7c681ec3c78
Status: RESOLVED → VERIFIED
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.