Gradient encode/decode does not validate that the serialized stops were sorted

• Component: Platform
• Status: RESOLVED FIXED

[Sam Weinig]

The Gradient encoder/decoder currently encodes the stopsSorted bit and on decode assumes that bit can be trusted without verifying validating that in fact the stops are sorted.

[Sam Weinig]

I don't believe there is any current issue with this, as it looks like the underlying graphics libraries do their own sort of the stops (which begs the question why are we sorting them), but leaving the object in this invalid state is non-the-less a bad idea.

[Sam Weinig]

Created attachment 447728 [details]
Patch

[EWS]

Committed r287324 (245474@main): <https://commits.webkit.org/245474@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447728 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86777463>

[Darin Adler]

Comment on attachment 447728 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=447728&action=review

Sure does look nice

> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:234
> +    Gradient::ColorStopVector result;
> +    result.reserveInitialCapacity(stops.size());
>      for (auto& stop : stops)
> -        gradient.addColorStop({ stop.offset, style.colorByApplyingColorFilter(stop.color) });
> +        result.uncheckedAppend({ stop.offset, style.colorByApplyingColorFilter(stop.color) });
> +    return result;

Seems like we could use Vector::map here and make this a bit more elegant

> Source/WebCore/rendering/svg/RenderSVGResourceLinearGradient.cpp:58
> +        Gradient::LinearData { startPoint(m_attributes), endPoint(m_attributes) },
> +        ColorInterpolationMethod { ColorInterpolationMethod::SRGB { }, AlphaPremultiplication::Unpremultiplied },

Can any of the type names be omitted? Maybe it would make the code more confusing, but I like the idea of making things a little more terse

> Source/WebCore/rendering/svg/RenderSVGResourceRadialGradient.cpp:69
> +        Gradient::RadialData { focalPoint(m_attributes), centerPoint(m_attributes), focalRadius(m_attributes), radius(m_attributes), 1 },
> +        ColorInterpolationMethod { ColorInterpolationMethod::SRGB { }, AlphaPremultiplication::Unpremultiplied },

Ditto.

[Sam Weinig]

(In reply to Darin Adler from comment #5)
> Comment on attachment 447728 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447728&action=review
> 
> Sure does look nice
> 
> > Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:234
> > +    Gradient::ColorStopVector result;
> > +    result.reserveInitialCapacity(stops.size());
> >      for (auto& stop : stops)
> > -        gradient.addColorStop({ stop.offset, style.colorByApplyingColorFilter(stop.color) });
> > +        result.uncheckedAppend({ stop.offset, style.colorByApplyingColorFilter(stop.color) });
> > +    return result;
> 
> Seems like we could use Vector::map here and make this a bit more elegant

Alas, I couldn't figure out how to make Vector::map() work with ColorStopVector due to the inline capacity. ColorStopVector == Vector<GradientColorStop, 2>.

> 
> > Source/WebCore/rendering/svg/RenderSVGResourceLinearGradient.cpp:58
> > +        Gradient::LinearData { startPoint(m_attributes), endPoint(m_attributes) },
> > +        ColorInterpolationMethod { ColorInterpolationMethod::SRGB { }, AlphaPremultiplication::Unpremultiplied },
> 
> Can any of the type names be omitted? Maybe it would make the code more
> confusing, but I like the idea of making things a little more terse
> 
> > Source/WebCore/rendering/svg/RenderSVGResourceRadialGradient.cpp:69
> > +        Gradient::RadialData { focalPoint(m_attributes), centerPoint(m_attributes), focalRadius(m_attributes), radius(m_attributes), 1 },
> > +        ColorInterpolationMethod { ColorInterpolationMethod::SRGB { }, AlphaPremultiplication::Unpremultiplied },
> 
> Ditto.

Yeah, not sure why I added the extra ColorInterpolationMethod in there. Will remove.

[Sam Weinig]

> > > Source/WebCore/rendering/svg/RenderSVGResourceLinearGradient.cpp:58
> > > +        Gradient::LinearData { startPoint(m_attributes), endPoint(m_attributes) },
> > > +        ColorInterpolationMethod { ColorInterpolationMethod::SRGB { }, AlphaPremultiplication::Unpremultiplied },
> > 
> > Can any of the type names be omitted? Maybe it would make the code more
> > confusing, but I like the idea of making things a little more terse
> > 
> > > Source/WebCore/rendering/svg/RenderSVGResourceRadialGradient.cpp:69
> > > +        Gradient::RadialData { focalPoint(m_attributes), centerPoint(m_attributes), focalRadius(m_attributes), radius(m_attributes), 1 },
> > > +        ColorInterpolationMethod { ColorInterpolationMethod::SRGB { }, AlphaPremultiplication::Unpremultiplied },
> > 
> > Ditto.
> 
> Yeah, not sure why I added the extra ColorInterpolationMethod in there. Will
> remove.

Will resolve these in https://bugs.webkit.org/show_bug.cgi?id=234583

[Darin Adler]

Comment on attachment 447728 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=447728&action=review

>>> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:234
>>> +    return result;
>> 
>> Seems like we could use Vector::map here and make this a bit more elegant
> 
> Alas, I couldn't figure out how to make Vector::map() work with ColorStopVector due to the inline capacity. ColorStopVector == Vector<GradientColorStop, 2>.

We can enhance Vector::map to make it a template that takes a type for the destination vector that defaults to the type of the source vector.

[Sam Weinig]

(In reply to Darin Adler from comment #8)
> Comment on attachment 447728 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447728&action=review
> 
> >>> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:234
> >>> +    return result;
> >> 
> >> Seems like we could use Vector::map here and make this a bit more elegant
> > 
> > Alas, I couldn't figure out how to make Vector::map() work with ColorStopVector due to the inline capacity. ColorStopVector == Vector<GradientColorStop, 2>.
> 
> We can enhance Vector::map to make it a template that takes a type for the
> destination vector that defaults to the type of the source vector.

https://bugs.webkit.org/show_bug.cgi?id=234683