524214 - XML wildcard-attribute assignment crashes

Status: VERIFIED FIXED

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

Description

[Steven Johnson]

Compile and run

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

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

Comment 1

[Steven Johnson]

Attachment: 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

Comment 2

[Brent Baker]

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?

Comment 3

[Edwin Smith]

Comment on attachment 408121 [details] [diff] [review]
Patch

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

Comment 4

[Steven Johnson]

pushed as changeset:   2917:d2de58311745

Comment 5

[Brent Baker]

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>

Comment 6

[Steven Johnson]

A better question is what you get in FP10, since that's the behavior we want

Comment 7

[Steven Johnson]

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

Comment 8

[Brent Baker]

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

Comment 9

[Steven Johnson]

Attachment: Patch #2

Fixes the assert/crash and matches firefox output.

Comment 10

[Brent Baker]

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

Comment 11

[Steven Johnson]

pushed as changeset changeset:   2964:8b93b0b96806

leaving bug open pending investigation of the inserted space

Comment 12

[Brent Baker]

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">

Comment 13

[Steven Johnson]

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

Comment 14

[Brent Baker]

Attachment: testcase

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

Comment 15

[Steven Johnson]

Attachment: Fix for rogue space insertion

Fixes the issue with toString being different afterwards.

Comment 16

[Brent Baker]

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

[Brent Baker]

Attachment: Updated testcase

update testcase to match now expected behavior

Comment 18

[Steven Johnson]

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

Comment 19

[Brent Baker]

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">

Comment 20

[Steven Johnson]

(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.

Comment 21

[Steven Johnson]

pushed to redux as changeset:   3016:5ec7389cf24e

declaring this fixed -- if we want to match (incorrect!) spidermonkey output let's open a new bug.