Bug 61882 - Some paths can create an XSSFColor instance with a null CTColor reference
Summary: Some paths can create an XSSFColor instance with a null CTColor reference
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: XSSF (show other bugs)
Version: 3.17-FINAL
Hardware: All All
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-12-09 19:38 UTC by Greg Woolsey
Modified: 2017-12-11 17:33 UTC (History)
0 users



Attachments
sample workbook with cell styles without colors (18.37 KB, application/vnd.openxmlformats-officedocument.spreadsheetml.sheet)
2017-12-09 19:38 UTC, Greg Woolsey
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Woolsey 2017-12-09 19:38:02 UTC
Created attachment 35596 [details]
sample workbook with cell styles without colors

The attached workbook, saved from Excel 2016, contains a cell style with a fill with a pattern but no background or foreground color.

Currently, POI will happily return an XSSFColor instance for this style's foreground and background color, with null for the CTColor field.  This will then lead to later NPEs in unexpected places, of course.

Since this is a normal and valid way to construct fills, and probably other format features, I don't think just checking for a null constructor parameter and throwing an exception is sufficient.  We need to replace constructors that take a CTColor object directly with a factory method that can check the input and return null if needed instead of an invalid XSSFColor object.
Comment 1 Greg Woolsey 2017-12-09 20:02:04 UTC
My internal bug report appears to be against an older version of POI - I'm unable to reproduce this with a unit test making the same call as the error my user reported.

However, I see we do have a lot of code where an inline null check or similar is done on the CTColor argument to decide whether to return null or create an XSSFColor instance.  That pattern is definitely safer as a factory, so I'll still investigate that.
Comment 2 Greg Woolsey 2017-12-11 17:33:55 UTC
added factory and deprecated CTColor based constructors in r1817796.