294137 - Mozilla accepts illegal syntax on transform attribute

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Holger Will]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050512 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050512 Firefox/1.0+

in a transform attribute when leaving out the last bracked, e.g:
transform="translate(100 100" or transform="translate(100 100) scale(0.5 0.5"
mozilla performs the transformation anyways, causing possible incompatibility
with excisting implementations.

Reproducible: Always

Steps to Reproduce:
1.add a correct transformation to an svg element
2.delete the last bracked
3.

Actual Results:  
everything still works fine

Expected Results:  
cause an error, or at least dont apply the transformation

Comment 1

[Holger Will]

Attachment: file with illegal transform attribute

Comment 2

[Boris Zbarsky [:bzbarsky]]

imo we should ship this if it's easy before shipping SVG in a release...

Comment 3

[Jonathan Watt [:jwatt]]

We can fix this after we branch if necessary. The problem area is:

 
http://lxr.mozilla.org/seamonkey/source/content/svg/content/src/nsSVGTransformList.cpp#128

and if anyone wants to work on this now, the relevant part of the spec is:

  http://www.w3.org/TR/SVG/coords.html#TransformAttribute

Comment 4

[Holger Will]

i'm not sure, but in
http://lxr.mozilla.org/seamonkey/source/content/svg/content/src/nsSVGTransformList.cpp#128
shouldnt it be enough to add ")" to delimiters3

143   const char* delimiters3 = "\x20\x9\xD\xA,";
>>
143   const char* delimiters3 = "\x20\x9\xD\xA,)";

Comment 5

[Keith]

Attachment: Reworked transform parsing into a descent parser to fix this and other syntax problems according to the 1.1 spec.

This patch is dependent on the patch added for 338142 -- dependent on the new superclass nsSVGDataParser.

This patch detects the case below as being invalid:
  transform="rotate(20"

Other cases that are detected and marked as invalid:
 transform="translate -10, -10)  scale 2)"
 transform="translate(-10, -10)  scale 2)"
 transform="translate 10 40"

Comment 6

[Keith]

The patch submitted for 294137 has a dependency on the patch submitted for 338142.  Can some one with the authorization make this known by updating the field "Bug 294137 depends on:" with bug number 338142???  

Comment 7

[Robert Longson [:longsonr]]

Requested dependency from comment 6 added.

Comment 8

[Keith]

Attachment: Updated patch to reflect changes from bug 338142

Updated patch to reflect changes made in bug 338142.

Comment 9

[tor]

Comment on attachment 224866 [details] [diff] [review]
Updated patch to reflect changes from bug 338142

Besides some overly clever array initialization, which is debatable, looks good.

Comment 10

[Robert Longson [:longsonr]]

Comment on attachment 224866 [details] [diff] [review]
Updated patch to reflect changes from bug 338142

>+//----------------------------------------------------------------------
>+nsresult nsSVGTransformListParser::GetTransformToken(nsIAtom** aKeyatom, PRBool aAdvancePos)
>+{
>+
>+  if (!(mTokenType == OTHER) || (*mTokenPos == '\0') ) 
>+     return NS_ERROR_FAILURE;
>+
>+  char* keyword;
>+  char buf[10];
>+  char* rest = strncpy(buf, mTokenPos, 10);
>+  const char* delimiters1 = "\x20\x9\xD\xA,(";
>+  keyword = nsCRT::strtok(rest, delimiters1, &rest);
>+

rest/buf may not be nul terminated if mTokenPos has 10 characters or more in. I think strtok expects a nul terminated string. Is this a problem?

I'm not sure why rest is used as it is the same as buf.

Comment 11

[tor]

(In reply to comment #10)
> rest/buf may not be nul terminated if mTokenPos has 10 characters or more in. I
> think strtok expects a nul terminated string. Is this a problem?

Hmm, yes.  Since the longest token we expect is 9 characters, we should have a buffer of 11 characters and force the last character to \0.  That way we can prevent accidently passing something "translateBla".

> I'm not sure why rest is used as it is the same as buf.

In addition to removing rest as Robert mentiones, use NS_ARRAY_LENGTH to generate the size passed to strncpy.

Comment 12

[Keith]

Attachment: Suggested changes made

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+{
+
+  if (!(mTokenType == OTHER) || (*mTokenPos == '\0') ) 

I don't much like this style of a blank line after the function open brace. Also, lose the unnecessary parens and convert ! == to !=

+    if ((mTokenPos != 0) && (nsCRT::strlen(mTokenPos) > 0 )) {

here too, and elsewhere

> aKeyatom

aKeyAtom

+  while (IsTokenWspStarter()) {
+    ENSURE_MATCHED(MatchWsp());
+  }

+  while (IsTokenCommaWspStarter()) {
+    ENSURE_MATCHED(MatchCommaWsp());
+  }

Create helpers for these, as they get repeated a lot?

I think you should also shrink the code by having a helper that matches between N and M comma-separated floats, putting them into an array.

Is it really intended to treat "translate(0,)" as translate(0,0)? Is that in fact what this does, or is the second float actually uninitialized?

+  nsCOMPtr<nsIDOMSVGTransform> transform;
+  NS_NewSVGTransform(getter_AddRefs(transform));
+
+  transform->SetSkewX(angle);
+
+  mTransform->AppendObject(transform);

You could also have a helper that creates a new transform object and adds it to mTransform, returning a weak pointer to the new transform object. Don't forget check for OOM, too.

Comment 14

[tor]

Attachment: address review comments

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 238311 [details] [diff] [review]
address review comments

nice

Comment 16

[tor]

Checked in.