XML wildcard-attribute assignment crashes

VERIFIED FIXED in flash10.1

Status

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

People

(Reporter: Steven Johnson, Assigned: Steven Johnson)

Tracking

unspecified
flash10.1
Bug Flags:
in-testsuite +
flashplayer-triage +

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
Compile and run

   var e:XML = <x><c/></x>;
   e.@* = 1;

And watch the crash. Recent injection from String/XML changes.
(Assignee)

Comment 1

9 years ago
Created attachment 408121 [details] [diff] [review]
Patch

the issue was that a call to internString() was added to E4XNode::setQName, without a null check. trivial fix.

remainder of patch is adding isAnyName() calls to remove asserts [doesn't change behavior], and some driveby whitespace fixes
Attachment #408121 - Flags: review?(edwsmith)
(Assignee)

Updated

9 years ago
Priority: -- → P2
Target Milestone: --- → flash10.1

Comment 2

9 years ago
What is the expected output of this? Looks like older avmshell would not fail with the above code, but would fail as soon as you tried to trace the XML.

This is the current output with the patch if you trace the XML:
<x (null)="1" xmlns="null">
  <c/>
</x>

Is it correct to add a null attribute to /x/ when it did not previously exist?
Flags: in-testsuite?
Flags: flashplayer-triage+

Updated

9 years ago
Attachment #408121 - Flags: review?(edwsmith) → review+

Comment 3

9 years ago
Comment on attachment 408121 [details] [diff] [review]
Patch

you can compare with spidermonkey if interpretation of the E4X spec is in question.
(Assignee)

Comment 4

9 years ago
pushed as changeset:   2917:d2de58311745

Comment 5

9 years ago
This is what I get running the code in firefox:

--> var e = <x><c/></x>;
--> e
<x><c/></x>
--> e.@* = 1;
1
--> e
<x><c/></x>

Just to test I added an attribute and it looks like they do not handle wildcard replacement at all.

--> var e = <x bar="d"><c/></x>;
--> e
<x bar="d"><c/></x>
--> e.@* = 1;
1
--> e
<x bar="d"><c/></x>
--> e.@bar = 1;
1
--> e
<x bar="1"><c/></x>
(Assignee)

Comment 6

9 years ago
A better question is what you get in FP10, since that's the behavior we want
(Assignee)

Comment 7

9 years ago
Technically, this bug is fixed (no longer crashes) but we may need to confirm the behavior is identical to FP10 before closing.

Comment 8

9 years ago
Assertion in debug builds running the sample code:
Assertion failed: "((an->getQName(&nam, publicNS)))" ("c:/buildbot/tamarin-redux
/windows/tamarin-redux/core/XMLObject.cpp":1277)avmplus crash: exception 0x80000
003 occurred
Writing minidump crash log to avmplusCrash.dmp
(Assignee)

Comment 9

9 years ago
Created attachment 409539 [details] [diff] [review]
Patch #2

Fixes the assert/crash and matches firefox output.
Attachment #408121 - Attachment is obsolete: true
Attachment #409539 - Flags: review?(wsharp)

Updated

9 years ago
Attachment #409539 - Flags: review?(wsharp) → review+

Comment 10

9 years ago
Comment on attachment 409539 [details] [diff] [review]
Patch #2

I am not 100% sure where in the patch this is happening but when doing a wildcard attribute assignment on the XML when there are no attributes, the node is modified so that when you get the node it now contains a space. 

Example:
   var e:XML = <x><c/></x>;
   e.@* = 1;

This will trace out the var e with "<x >" instead of "<x>", this makes e != to the original value, which should not have been modified since there were no attributes to modify
(Assignee)

Comment 11

9 years ago
pushed as changeset changeset:   2964:8b93b0b96806

leaving bug open pending investigation of the inserted space

Comment 12

9 years ago
Trying to understand the intent of the spec with regards to wildcard attribute
assignment...

Spidermonkey does not handle wildcard assignment at all, attributes are never
modified.

TC handles wildcard assignment of single attribute.
<x foo="0">; x.@*=1; --> <x foo="1">

TC does not handle multiple attributes on a node, only maintains first
attribute, other attributes on node are lost.
<x foo="0" bar="0">; x.@*=1; --> <x foo="1">
(Assignee)

Comment 13

9 years ago
since it was totally broken before, current behavior is merely suboptimal. still would be nice to get it 100% right.

Comment 14

9 years ago
Created attachment 409728 [details] [diff] [review]
testcase

I would assume that all of the tests in this patch should function properly

Updated

9 years ago
Attachment #409728 - Flags: review?(stejohns)
(Assignee)

Updated

9 years ago
Attachment #409728 - Flags: review?(stejohns) → review+
(Assignee)

Comment 15

9 years ago
Created attachment 410656 [details] [diff] [review]
Fix for rogue space insertion

Fixes the issue with toString being different afterwards.
Attachment #410656 - Flags: review?(wsharp)

Updated

9 years ago
Attachment #410656 - Flags: review?(wsharp) → review+

Comment 16

9 years ago
Looks like there is still an assertion happening in debug builds:

$ $AVM e4x/Regress/regress-524214.abc
Assertion failed:
"((!isAnyName() && !isRtname()))" ("c:\\hg\\clean\\core\\Multiname.h":82)avmplus crash: exception 0x80000003 occurred

Comment 17

9 years ago
Created attachment 410802 [details] [diff] [review]
Updated testcase

update testcase to match now expected behavior
Attachment #409728 - Attachment is obsolete: true
(Assignee)

Comment 18

9 years ago
oops, I neglected to test with the new testcase. Fix looks simple, I'll slipstream it in with the testcase.

Comment 19

9 years ago
And the plan is to match the incorrect spidermonkey handling of multiple attributes on a node?


<x foo="0" bar="0">; 
x.@*=1; 
Produces --> <x foo="1">
and NOT
Produces --> <x foo="1" bar="1">
(Assignee)

Comment 20

9 years ago
(In reply to comment #19)
> And the plan is to match the incorrect spidermonkey handling of multiple
> attributes on a node?
> 
> 
> <x foo="0" bar="0">; 
> x.@*=1; 
> Produces --> <x foo="1">
> and NOT
> Produces --> <x foo="1" bar="1">

Nope, wasn't part of the plan. This bug is "fix the crash", not "make it match spidermonkey" -- if the latter is desirable, IMHO we should open a new bug.
(Assignee)

Comment 21

9 years ago
pushed to redux as changeset:   3016:5ec7389cf24e

declaring this fixed -- if we want to match (incorrect!) spidermonkey output let's open a new bug.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Status: RESOLVED → VERIFIED
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.