For pattern colorspace, its base colorspace should not be a pattern colorspace

For pattern colorspace, its base colorspace should not be a pattern colorspace

BUG=405588
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/4fb24f3

[Bo Xu, 2014-08-21 21:11:04]

Hi Tom, please review this one, thanks!

PDF reference 8.7.3.3 says the underlying color space cannot be another Pattern
color space.

The buffer overflow is due to recursively using |pBuf->m_Comps|. If m_pBaseCs is
another color space, m_pBaseCS->GetRGB won't have this issue.

[Tom Sepez, 2014-08-21 21:17:15]

Good.  I was looking at this the other day as it turns out, and the prohibition
you are describing makes sense.

But, can we enforce this when we set the m_pBaseCS member?  It seems a shame to
have a chain of these things in memory if the spec doesn't allow it.  If we
stopped this during the creation of the chain, we could simply assert that this
is not a pattern in this code.

[Bo Xu, 2014-08-21 21:43:06]

On 2014/08/21 21:17:15, Tom Sepez wrote:
> Good.  I was looking at this the other day as it turns out, and the
prohibition
> you are describing makes sense.
> 
> But, can we enforce this when we set the m_pBaseCS member?  It seems a shame
to
> have a chain of these things in memory if the spec doesn't allow it.  If we
> stopped this during the creation of the chain, we could simply assert that
this
> is not a pattern in this code.

Totally agree. Now stopping loading the base colorspace when it's a pattern.

[Tom Sepez, 2014-08-21 22:04:22]

https://codereview.chromium.org/496883002/diff/20001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp (right):

https://codereview.chromium.org/496883002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:861: PatternValue* pvalue =
(PatternValue*)pBuf;
can we add assert(m_pBaseCS->GetFamily() != PDFCS_PATTERN); here?

[Bo Xu, 2014-08-21 22:14:41]

https://codereview.chromium.org/496883002/diff/20001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp (right):

https://codereview.chromium.org/496883002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:861: PatternValue* pvalue =
(PatternValue*)pBuf;
On 2014/08/21 22:04:22, Tom Sepez wrote:
> can we add assert(m_pBaseCS->GetFamily() != PDFCS_PATTERN); here?

Done.

[Tom Sepez, 2014-08-21 22:16:04]

lgtm

[Bo Xu, 2014-08-21 22:19:07]

Committed patchset #3 manually as 4fb24f3 (presubmit successful).

[Tom Sepez, 2014-08-21 22:29:53]

Looking through the file fpdf_page_colors.cpp, there are a bunch of places where
the anti-pattern

    m_pAltCS->GetRGB(results, R, G, B);
    return TRUE;

occurs.  A follow-up CL should probably change all these to

    return m_pAltCS->GetRGB(results, R, G, B);

Though I'd like to be sure that there is sufficient testing when making the
change.

[Bo Xu, 2014-08-21 22:33:06]

On 2014/08/21 22:29:53, Tom Sepez wrote:
> Looking through the file fpdf_page_colors.cpp, there are a bunch of places
where
> the anti-pattern
> 
>     m_pAltCS->GetRGB(results, R, G, B);
>     return TRUE;
> 
> occurs.  A follow-up CL should probably change all these to
> 
>     return m_pAltCS->GetRGB(results, R, G, B);
> 
> Though I'd like to be sure that there is sufficient testing when making the
> change.

I agree with the sufficient test. Are we going to have some sort of bitmap
comparison for quality test? This maybe good to land with that.