904158 - Animation of mapped attributes on SVG elements can end up hitting string-handling/termination issues

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

As Dirk noted in bug 896050 comment 19 and bug 896050 comment 20, it looks like we're mis-handling strings in SMIL animations of SVG string-valued mapped attributes.

I'm filing this bug on fixing that underlying issue.  Assigning to myself, as I'm hoping to work on this in the next few days. (since this blocks the landing of bug 896050)

Comment 1

[Daniel Holbert [:dholbert]]

Dirk, it sounds like (in bug 896050 comment 20) you have a standalone testcase that reproduces this, in current mozilla-central builds (no need for your filter patch) -- if so, could you attach that here? I tried to make a testcase, using long "filter" and "fill" URLs, but I couldn't. (So I'm just using the modified mochitest for now -- but ultimately I'd love to be able to check in the fix for this with a simple, targeted, standalone testcase)

Comment 2

[Daniel Holbert [:dholbert]]

[I'm debugging this with Dirk's patch applied from his Try run from bug 896050 comment 10, and using a modified version of test_smilMappedAttrFromTo.xhtml which just animates filter from "#idA to #idB" and does nothing else]

So when we hit nsSMILMappedAttribute::SetAnimValue()'s call to "valueToString", we end up with a buffer that happens to be bigger than we need, by one PRUnichar value. (The fact that it's off by exactly one isn't significant -- it could be off by any amount. It's inexact because we use a doubling algorithm when allocating extra data for string-appending.)

In particular -- I've got:
> (gdb) p valStr.mLength
> 251
> (gdb) p *((nsStringBuffer*)valStr.mData - 1)
> {
>   mRefCount = 1, 
>   mStorageSize = 506
> }
So we've got 506 bytes of storage allocated, when we really only need 251 PRUnichars + 1 null-terminator = 252 PRUnichars. (which would be 252 * 2 = 504 bytes)

So we've got one bonus PRunichar at the end of our buffer, which contains random data.

We end up sticking this nsStringBuffer in our element's property-table, and then 
when we hit ParseMappedAttrAnimValueCallback, we pull it out of the property-table and wrap it in a nsCheapString.  And here's the key part -- nsCheapString's constructor **assumes the passed-in nsStringBuffer is exactly the right length**:
> 63   nsCheapString(nsStringBuffer* aBuf)
> 64   {
> 65     if (aBuf)
> 66       aBuf->ToString(aBuf->StorageSize()/2 - 1, *this);
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsAttrValue.h#66

That is -- it's taking our buffer's allocated size, dividing it by 2 (to convert from bytes to PRUnichars), and subtracting one (to account for the null-terminator), and using the result as the string-length. And in our case, that is *wrong* -- our buffer has some extra space, so this gives us a string that's too long.

Comment 3

[Daniel Holbert [:dholbert]]

This explains the assertion that we hit during Dirk's try run:
> "###!!! ASSERTION: data should be null terminated: 'data[len] == PRUnichar(0)', file ../../../../xpcom/string/src/nsSubstring.cpp

(In my case, data[len] == data[252] == the bonus/bogus PRUnichar at the end of our buffer.
If we were checking data[251], we'd find the *actual* null-terminator.)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

OK, I've got a testcase -- I was stupidly forgetting to use attributeType="XML" before, so I wasn't hitting this. (This only affects mapped attributes -- not their CSS counterparts.  This is because we rely on the nsStringBuffer-passing described above for mapped attributes, but for CSS properties we instead call GetSMILOverrideStyle() and directly insert the string into the returned CSS declaration.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Here's a fix, with a reftest.

Given that our temporary storage is *just* a nsStringBuffer, and nsStringBuffer::FromString() needs to know the string-length (which nsCheapString derives from the buffer's size), we need to make sure the buffer is exactly the right size (and *no larger*).

We can guarantee that by extending nsCSSValue::BufferFromString slightly. It already has logic to use an existing buffer or allocate a new one -- this patch just lets us force it to allocate a new one when the size is bigger than it needs to be, for callers who care about that.

(I verified that the reftest passes locally with the patch, & fails without the patch. It also asserts (the assertion from bug 896050 comment 11) without the patch.)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v1a

(just fixed two comment typos from prev. patch)

Comment 7

[Daniel Holbert [:dholbert]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=cae2d7bb3f70

Comment 8

[Robert Longson [:longsonr]]

Does this need to change too?

http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILMappedAttribute.cpp#103

Comment 9

[Robert Longson [:longsonr]]

Never mind me, that is what you changed.

Comment 10

[Daniel Holbert [:dholbert]]

Yup. :)

Also: Sorry, Comment 7's Try run was missing unit tests in its trychooser message.
Pushed again, now asking for reftests/crashtests/mochitests:
  https://tbpl.mozilla.org/?tree=Try&rev=1a867785f92b

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 789315 [details] [diff] [review]
fix v1a

We discussed earlier that I think the code that assumes that a string buffer's logical length matches its allocated length is the code that's at fault.  I don't think we should add new assumptions unless we see performance problems from not doing so, so we just need a strlen call in the code that's converting nsStringBuffer to a string.

(Though that has the risk of embedded nulls -- I hope there's not a chance of those here, though.)

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: fix v2

OK -- this version uses NS_strlen (which takes a const PRUnichar*), and also grabs the allocated length as an upper-limit, for good measure (matching the nsCheapString length-measurement from the end of comment 2).

Comment 13

[Robert Longson [:longsonr]]

If the string wasn't null terminated won't the NS_strlen call read beyond the memory allocated to the buffer? All your checking and asserts happen after that.

Comment 14

[Daniel Holbert [:dholbert]]

What I really want is NS_strnlen, to guarantee that the strlen operation itself won't run past the end of the buffer.  But absent that, I don't think it really matters which order I do the strlen vs. allocated-size-check. If NS_strlen is going to read past the end of the buffer, then I won't find out until *after* I've gotten its return value back and compared it against the buffer size.

Good news, though -- the string *should* be null-terminated, because nsSMILMappedAttribute wraps the animated value in "nsString()" (which null-terminates) before putting it in the property table:
> 102   nsStringBuffer* valStrBuf =
> 103     nsCSSValue::BufferFromString(nsString(valStr)).get();
> 104   nsresult rv = mElement->SetProperty(SMIL_MAPPED_ATTR_ANIMVAL,
> 105                                       attrName, valStrBuf,
> 106                                       ReleaseStringBufferPropertyValue);

So we shouldn't have anything to worry about. The allocated size check is just a safeguard in case that fails, to limit the amount of damage we'll do.

Comment 15

[Daniel Holbert [:dholbert]]

NOTE: I was going to post a followup to add patch v1's other nsSMILMappedAttribute tweaks (the only functional tweak being a null-check of BufferFromString to be consistent with most other callers), but then I noticed that BufferFromString is actually infallible, so there's no need to null-check its result. So instead of posting a followup, I filed and fixed bug 907547, to remove the null-checks from the other callers.

Comment 16

[Daniel Holbert [:dholbert]]

[gentle review-ping; krit's filter patch in bug 896050 is just about ready to land, but it can't land until this is fixed]

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 793295 [details] [diff] [review]
fix v2

>+  nsStringBuffer* animValBuf = static_cast<nsStringBuffer*>(aPropertyValue);
>+  if (!animValBuf)
>+    return;

Is this null check really needed?  (I'm not sure, but just asking.)

>+  PRUnichar* animValBufData = static_cast<PRUnichar*>(animValBuf->Data());
>+  uint32_t logicalStringLen = NS_strlen(animValBufData);
>+  // SANITY CHECK: In case the string buffer wasn't correctly
>+  // null-terminated, let's check the allocated size, too, and make sure we
>+  // don't read further than that. (Note that StorageSize() is in units of
>+  // bytes, so we have to convert that to units of PRUnichars, and subtract
>+  // 1 for the null-terminator.)
>+  uint32_t allocStringLen =
>+    (animValBuf->StorageSize() / sizeof(PRUnichar)) - 1;
>+
>+  MOZ_ASSERT(logicalStringLen <= allocStringLen,
>+             "The string in our string buffer wasn't null-terminated!!");
>+
>+  nsDependentString animValStr;

Please declare this as nsString.  The code below doesn't depend at all on the type of nsString declared, so you should just use the base rather than one of the special constructor types like nsDependentString.



And could you add a big warning comment above the definition of class nsCheapString explaining how it's dangerous?

r=dbaron with that

(And aren't you supposed to be on vacation?)

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] (needinfo? me; away Aug 28 - Sep 3) from comment #17)
> Is this null check really needed?  (I'm not sure, but just asking.)

I'll double-check, but now that you mention it, I think it's probably not needed & can go.

The old code gracefully handles null values (due to how nsCheapString works), and I was thinking we needed that for "no animated value" cases, but now I'm realizing that this function just won't be invoked in those cases.

So I'll probably drop that before landing. Thanks!

> >+  nsDependentString animValStr;
> 
> Please declare this as nsString.  The code below doesn't depend at all on
> the type of nsString declared, so you should just use the base rather than
> one of the special constructor types like nsDependentString.

My intention was to reduce the chances of needless copying -- but after re-reading the nsStringBuffer::ToString documentation, it seems like that method avoid copying by default, if the destination string-type supports sharing (as nsString does).

So: I'll switch to nsString as you suggest.

> And could you add a big warning comment above the definition of class
> nsCheapString explaining how it's dangerous?

Will do.

> r=dbaron with that
> 
> (And aren't you supposed to be on vacation?)

Thanks! And yes, I suppose I am. :) I didn't want to allow krit's filter-animation work to stall or bitrot just on account of that, though.

Anyway: hoping to address review comments and land this sometime tomorrow.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: fix v3 (review comments addressed) [r=dbaron]

Here's the patch w/ review comments addressed. Inbound's closed, so I'm posting this as checkin-needed.  Carrying forward r=dbaron.

(krit, feel free to import this locally and base your patches on top of it, so you can get a Try run going.)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/065aa28e30d3

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/065aa28e30d3