[CSS Grid Layout] ASSERTION FAILED: repetitions > 0

[CSS Grid Layout]	ASSERTION FAILED: repetitions > 0

If the author specifies an absurdly large number of repetitions it might
happen that the double value contained in the CSSParserValue does not fit in
a size_t, so it is clamped down to 0 during the type casting.

From now on we limit the value of repetitions to kGridMaxTracks (1 million)
before casting the double value to a size_t.

BUG=425001
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184154

[svillar, 2014-10-21 14:55:38]

svillar@igalia.com changed reviewers:
+ esprehn@chromium.org, inferno@chromium.org, jchaffraix@chromium.org

[svillar, 2014-10-21 14:57:41]

Sending for review

[Julien - ping for review, 2014-10-21 15:33:12]

https://codereview.chromium.org/645793005/diff/1/Source/core/css/parser/CSSPr...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/645793005/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:3582:
ASSERT_WITH_SECURITY_IMPLICATION(arguments->valueAt(0)->fValue > 0);
I don't have this ASSERT on my tree.

https://codereview.chromium.org/645793005/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:3583: size_t repetitions =
std::min<double>(arguments->valueAt(0)->fValue, kGridMaxTracks);
Why don't we just use clampTo<size_t>(argument->valueAt(0)->fValue, 0,
kGridMaxTracks)?

[svillar, 2014-10-21 15:48:49]

https://codereview.chromium.org/645793005/diff/1/Source/core/css/parser/CSSPr...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/645793005/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:3582:
ASSERT_WITH_SECURITY_IMPLICATION(arguments->valueAt(0)->fValue > 0);
On 2014/10/21 15:33:12, Julien Chaffraix - PST wrote:
> I don't have this ASSERT on my tree.

You should, I've just checked current master and it's there. Note that I'm
changing here repetitions by arguments->valueAt(0)->fValue in order to directly
use the parsed value.

https://codereview.chromium.org/645793005/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:3583: size_t repetitions =
std::min<double>(arguments->valueAt(0)->fValue, kGridMaxTracks);
On 2014/10/21 15:33:12, Julien Chaffraix - PST wrote:
> Why don't we just use clampTo<size_t>(argument->valueAt(0)->fValue, 0,
> kGridMaxTracks)?

You're totally right. Let's do it.

[Julien - ping for review, 2014-10-21 19:22:36]

lgtm

https://codereview.chromium.org/645793005/diff/1/Source/core/css/parser/CSSPr...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/645793005/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:3582:
ASSERT_WITH_SECURITY_IMPLICATION(arguments->valueAt(0)->fValue > 0);
On 2014/10/21 15:48:49, svillar wrote:
> On 2014/10/21 15:33:12, Julien Chaffraix - PST wrote:
> > I don't have this ASSERT on my tree.
> 
> You should, I've just checked current master and it's there. Note that I'm
> changing here repetitions by arguments->valueAt(0)->fValue in order to
directly
> use the parsed value.

Sigh, I was looking at the wrong place.

https://codereview.chromium.org/645793005/diff/20001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/645793005/diff/20001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:3582:
ASSERT_WITH_SECURITY_IMPLICATION(arguments->valueAt(0)->fValue > 0);
With clampTo, this ASSERT doesn't have security implications anymore as we won't
cast a negative float to a size_t.

Nit: I vote to downgrade it to a regular ASSERT.

[svillar, 2014-10-22 07:02:26]

https://codereview.chromium.org/645793005/diff/20001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/645793005/diff/20001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:3582:
ASSERT_WITH_SECURITY_IMPLICATION(arguments->valueAt(0)->fValue > 0);
On 2014/10/21 19:22:36, Julien Chaffraix - PST wrote:
> With clampTo, this ASSERT doesn't have security implications anymore as we
won't
> cast a negative float to a size_t.

I guess we shouldn't anyway because we have the
!validUnit(arguments->valueAt(0), FPositiveInteger) check above.
 
> Nit: I vote to downgrade it to a regular ASSERT.

Let's do it before landing.

[svillar, 2014-10-22 07:05:12]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2014-10-22 07:06:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/645793005/40001

[commit-bot: I haz the power, 2014-10-22 07:19:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-22 07:19:18]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[svillar, 2014-10-22 07:23:49]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2014-10-22 07:24:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/645793005/40001

[commit-bot: I haz the power, 2014-10-22 08:48:25]

Committed patchset #3 (id:40001) as 184154

[sof, 2014-10-23 09:39:11]

Just noting that CSSPropertyParserTest.GridTrackLimits now takes ~16 secs to run
locally; this doubles the overall running time of non-single-process unit tests.

It's a fair bit faster with Oilpan enabled, though.

[svillar, 2014-10-23 09:44:26]

On 2014/10/23 09:39:11, sof wrote:
> Just noting that CSSPropertyParserTest.GridTrackLimits now takes ~16 secs to
run
> locally; this doubles the overall running time of non-single-process unit
tests.
> 
> It's a fair bit faster with Oilpan enabled, though.

That's bizarre. In my local machines it takes ~300ms. Granted is slower than
other unit tests, but it's not even close to the figures you're reporting. IMO
there should be something wrong in your setup. Note that we're just adding 4 new
test cases. In my case the test went from ~270ms to ~300ms.

Could you try to profile it and check what's making it so slow?