Skip to content

Commit

Permalink
Bug 61882 - Some paths can create an XSSFColor instance with a null C…
Browse files Browse the repository at this point in the history 
…TColor reference

Protect against this in the future by introducing a factory method to create XSSFColor instances from a CTColor instance and the associated workbook style indexed color map.

If the CTColor instance is null, the factory returns null.  All callers already are prepared for a null instance, but many had their own null check on the CTColor object.  This centralizes that.

This also further forces the requirement for the indexed color map.  Any time a color is created, the workbook or styleTable is available in the same context, so passing this is extra parameter is trivial and allows XSSFColor to properly reference custom/themed indexed colors.

Did not remove any methods yet, only deprecated them.  Changed the signature to one internal test-only constructor.

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1817796 13f79535-47bb-0310-9956-ffa450edef68
  • Loading branch information
Greg Woolsey committed Dec 11, 2017
1 parent 185a66a commit 11e60eb
Show file tree
Hide file tree
Showing 23 changed files with 113 additions and 66 deletions.
40 changes: 20 additions & 20 deletions src/examples/src/org/apache/poi/xssf/usermodel/examples/CalendarDemo.java
View file
Expand Up @@ -140,7 +140,7 @@ private static Map<String, XSSFCellStyle> createStyles(XSSFWorkbook wb){
XSSFCellStyle style;
XSSFFont titleFont = wb.createFont();
titleFont.setFontHeightInPoints((short)48);
titleFont.setColor(new XSSFColor(new java.awt.Color(39, 51, 89)));
titleFont.setColor(new XSSFColor(new java.awt.Color(39, 51, 89), wb.getStylesSource().getIndexedColors()));
style = wb.createCellStyle();
style.setAlignment(HorizontalAlignment.CENTER);
style.setVerticalAlignment(VerticalAlignment.CENTER);
Expand All @@ -149,12 +149,12 @@ private static Map<String, XSSFCellStyle> createStyles(XSSFWorkbook wb){

XSSFFont monthFont = wb.createFont();
monthFont.setFontHeightInPoints((short)12);
monthFont.setColor(new XSSFColor(new java.awt.Color(255, 255, 255)));
monthFont.setColor(new XSSFColor(new java.awt.Color(255, 255, 255), wb.getStylesSource().getIndexedColors()));
monthFont.setBold(true);
style = wb.createCellStyle();
style.setAlignment(HorizontalAlignment.CENTER);
style.setVerticalAlignment(VerticalAlignment.CENTER);
style.setFillForegroundColor(new XSSFColor(new java.awt.Color(39, 51, 89)));
style.setFillForegroundColor(new XSSFColor(new java.awt.Color(39, 51, 89), wb.getStylesSource().getIndexedColors()));
style.setFillPattern(FillPatternType.SOLID_FOREGROUND);
style.setFont(monthFont);
styles.put("month", style);
Expand All @@ -165,64 +165,64 @@ private static Map<String, XSSFCellStyle> createStyles(XSSFWorkbook wb){
style = wb.createCellStyle();
style.setAlignment(HorizontalAlignment.LEFT);
style.setVerticalAlignment(VerticalAlignment.TOP);
style.setFillForegroundColor(new XSSFColor(new java.awt.Color(228, 232, 243)));
style.setFillForegroundColor(new XSSFColor(new java.awt.Color(228, 232, 243), wb.getStylesSource().getIndexedColors()));
style.setFillPattern(FillPatternType.SOLID_FOREGROUND);
style.setBorderLeft(BorderStyle.THIN);
style.setLeftBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89)));
style.setLeftBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89), wb.getStylesSource().getIndexedColors()));
style.setBorderBottom(BorderStyle.THIN);
style.setBottomBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89)));
style.setBottomBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89), wb.getStylesSource().getIndexedColors()));
style.setFont(dayFont);
styles.put("weekend_left", style);

style = wb.createCellStyle();
style.setAlignment(HorizontalAlignment.CENTER);
style.setVerticalAlignment(VerticalAlignment.TOP);
style.setFillForegroundColor(new XSSFColor(new java.awt.Color(228, 232, 243)));
style.setFillForegroundColor(new XSSFColor(new java.awt.Color(228, 232, 243), wb.getStylesSource().getIndexedColors()));
style.setFillPattern(FillPatternType.SOLID_FOREGROUND);
style.setBorderRight(BorderStyle.THIN);
style.setRightBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89)));
style.setRightBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89), wb.getStylesSource().getIndexedColors()));
style.setBorderBottom(BorderStyle.THIN);
style.setBottomBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89)));
style.setBottomBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89), wb.getStylesSource().getIndexedColors()));
styles.put("weekend_right", style);

style = wb.createCellStyle();
style.setAlignment(HorizontalAlignment.LEFT);
style.setVerticalAlignment(VerticalAlignment.TOP);
style.setBorderLeft(BorderStyle.THIN);
style.setFillForegroundColor(new XSSFColor(new java.awt.Color(255, 255, 255)));
style.setFillForegroundColor(new XSSFColor(new java.awt.Color(255, 255, 255), wb.getStylesSource().getIndexedColors()));
style.setFillPattern(FillPatternType.SOLID_FOREGROUND);
style.setLeftBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89)));
style.setLeftBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89), wb.getStylesSource().getIndexedColors()));
style.setBorderBottom(BorderStyle.THIN);
style.setBottomBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89)));
style.setBottomBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89), wb.getStylesSource().getIndexedColors()));
style.setFont(dayFont);
styles.put("workday_left", style);

style = wb.createCellStyle();
style.setAlignment(HorizontalAlignment.CENTER);
style.setVerticalAlignment(VerticalAlignment.TOP);
style.setFillForegroundColor(new XSSFColor(new java.awt.Color(255, 255, 255)));
style.setFillForegroundColor(new XSSFColor(new java.awt.Color(255, 255, 255), wb.getStylesSource().getIndexedColors()));
style.setFillPattern(FillPatternType.SOLID_FOREGROUND);
style.setBorderRight(BorderStyle.THIN);
style.setRightBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89)));
style.setRightBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89), wb.getStylesSource().getIndexedColors()));
style.setBorderBottom(BorderStyle.THIN);
style.setBottomBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89)));
style.setBottomBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89), wb.getStylesSource().getIndexedColors()));
styles.put("workday_right", style);

style = wb.createCellStyle();
style.setBorderLeft(BorderStyle.THIN);
style.setFillForegroundColor(new XSSFColor(new java.awt.Color(234, 234, 234)));
style.setFillForegroundColor(new XSSFColor(new java.awt.Color(234, 234, 234), wb.getStylesSource().getIndexedColors()));
style.setFillPattern(FillPatternType.SOLID_FOREGROUND);
style.setBorderBottom(BorderStyle.THIN);
style.setBottomBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89)));
style.setBottomBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89), wb.getStylesSource().getIndexedColors()));
styles.put("grey_left", style);

style = wb.createCellStyle();
style.setFillForegroundColor(new XSSFColor(new java.awt.Color(234, 234, 234)));
style.setFillForegroundColor(new XSSFColor(new java.awt.Color(234, 234, 234), wb.getStylesSource().getIndexedColors()));
style.setFillPattern(FillPatternType.SOLID_FOREGROUND);
style.setBorderRight(BorderStyle.THIN);
style.setRightBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89)));
style.setRightBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89), wb.getStylesSource().getIndexedColors()));
style.setBorderBottom(BorderStyle.THIN);
style.setBottomBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89)));
style.setBottomBorderColor(new XSSFColor(new java.awt.Color(39, 51, 89), wb.getStylesSource().getIndexedColors()));
styles.put("grey_right", style);

return styles;
Expand Down
6 changes: 3 additions & 3 deletions src/examples/src/org/apache/poi/xssf/usermodel/examples/WorkingWithRichText.java
View file
Expand Up @@ -37,17 +37,17 @@ public static void main(String[] args) throws Exception {

XSSFFont font1 = wb.createFont();
font1.setBold(true);
font1.setColor(new XSSFColor(new java.awt.Color(255, 0, 0)));
font1.setColor(new XSSFColor(new java.awt.Color(255, 0, 0), wb.getStylesSource().getIndexedColors()));
rt.applyFont(0, 10, font1);

XSSFFont font2 = wb.createFont();
font2.setItalic(true);
font2.setUnderline(XSSFFont.U_DOUBLE);
font2.setColor(new XSSFColor(new java.awt.Color(0, 255, 0)));
font2.setColor(new XSSFColor(new java.awt.Color(0, 255, 0), wb.getStylesSource().getIndexedColors()));
rt.applyFont(10, 19, font2);

XSSFFont font3 = wb.createFont();
font3.setColor(new XSSFColor(new java.awt.Color(0, 0, 255)));
font3.setColor(new XSSFColor(new java.awt.Color(0, 0, 255), wb.getStylesSource().getIndexedColors()));
rt.append(" Jumped over the lazy dog", font3);

cell.setCellValue(rt);
Expand Down
2 changes: 1 addition & 1 deletion src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFBorderFormatting.java
View file
Expand Up @@ -364,6 +364,6 @@ private short getIndexedColor(XSSFColor color) {
}

private XSSFColor getColor(CTBorderPr pr) {
return pr == null ? null : new XSSFColor(pr.getColor(), _colorMap);
return pr == null ? null : XSSFColor.from(pr.getColor(), _colorMap);
}
}
13 changes: 7 additions & 6 deletions src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFCellStyle.java
View file
Expand Up @@ -39,6 +39,7 @@ Licensed to the Apache Software Foundation (ASF) under one or more
import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTBorder;
import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTBorderPr;
import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTCellAlignment;
import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTColor;
import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTFill;
import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTFont;
import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTPatternFill;
Expand Down Expand Up @@ -755,7 +756,7 @@ public void setBorderTop(BorderStyle border) {
*/
@Override
public void setBottomBorderColor(short color) {
XSSFColor clr = new XSSFColor();
XSSFColor clr = XSSFColor.from(CTColor.Factory.newInstance(), _stylesSource.getIndexedColors());
clr.setIndexed(color);
setBottomBorderColor(clr);
}
Expand Down Expand Up @@ -865,7 +866,7 @@ public void setFillBackgroundColor(XSSFColor color) {
*/
@Override
public void setFillBackgroundColor(short bg) {
XSSFColor clr = new XSSFColor();
XSSFColor clr = XSSFColor.from(CTColor.Factory.newInstance(), _stylesSource.getIndexedColors());
clr.setIndexed(bg);
setFillBackgroundColor(clr);
}
Expand Down Expand Up @@ -900,7 +901,7 @@ public void setFillForegroundColor(XSSFColor color) {
*/
@Override
public void setFillForegroundColor(short fg) {
XSSFColor clr = new XSSFColor();
XSSFColor clr = XSSFColor.from(CTColor.Factory.newInstance(), _stylesSource.getIndexedColors());
clr.setIndexed(fg);
setFillForegroundColor(clr);
}
Expand Down Expand Up @@ -1027,7 +1028,7 @@ public void setIndention(short indent) {
*/
@Override
public void setLeftBorderColor(short color) {
XSSFColor clr = new XSSFColor();
XSSFColor clr = XSSFColor.from(CTColor.Factory.newInstance(), _stylesSource.getIndexedColors());
clr.setIndexed(color);
setLeftBorderColor(clr);
}
Expand Down Expand Up @@ -1082,7 +1083,7 @@ public void setQuotePrefixed(boolean quotePrefix) {
*/
@Override
public void setRightBorderColor(short color) {
XSSFColor clr = new XSSFColor();
XSSFColor clr = XSSFColor.from(CTColor.Factory.newInstance(), _stylesSource.getIndexedColors());
clr.setIndexed(color);
setRightBorderColor(clr);
}
Expand Down Expand Up @@ -1139,7 +1140,7 @@ public void setRotation(short rotation) {
*/
@Override
public void setTopBorderColor(short color) {
XSSFColor clr = new XSSFColor();
XSSFColor clr = XSSFColor.from(CTColor.Factory.newInstance(), _stylesSource.getIndexedColors());
clr.setIndexed(color);
setTopBorderColor(clr);
}
Expand Down
33 changes: 29 additions & 4 deletions src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFColor.java
View file
Expand Up @@ -32,6 +32,15 @@ public class XSSFColor extends ExtendedColor {
private final CTColor ctColor;
private final IndexedColorMap indexedColorMap;

/**
* @param color
* @param map
* @return null if color is null, new instance otherwise
*/
public static XSSFColor from(CTColor color, IndexedColorMap map) {
return color == null ? null : new XSSFColor(color, map);
}

/**
* Create an instance of XSSFColor from the supplied XML bean, with default color indexes
* @param color The {@link CTColor} to use as color-value.
Expand All @@ -47,7 +56,9 @@ public XSSFColor(CTColor color) {
* Create an instance of XSSFColor from the supplied XML bean, with the given color indexes
* @param color The {@link CTColor} to use as color-value.
* @param map The IndexedColorMap to use instead of the default one
* @deprecated 4.0.0 - use the factory {@link #from(CTColor, IndexedColorMap)} method instead to check for null CTColor instances. Make private eventually
*/
@Deprecated
public XSSFColor(CTColor color, IndexedColorMap map) {
this.ctColor = color;
this.indexedColorMap = map;
Expand All @@ -56,17 +67,31 @@ public XSSFColor(CTColor color, IndexedColorMap map) {
/**
* Create an new instance of XSSFColor, without knowledge of any custom indexed colors.
* This is OK for just transiently setting indexes, etc. but is discouraged in read/get uses
* @deprecated as of 4.0.0, we want to have the indexed map, and all calling contexts have access to it.
* @see #XSSFColor(IndexedColorMap)
* @see #from(CTColor, IndexedColorMap)
*/
@Deprecated
@Removal(version="4.1")
public XSSFColor() {
this(CTColor.Factory.newInstance(), null);
this(CTColor.Factory.newInstance(), new DefaultIndexedColorMap());
}

/**
* TEST ONLY - does not know about custom indexed colors
* new color with the given indexed color map
* @param colorMap
*/
public XSSFColor(IndexedColorMap colorMap) {
this(CTColor.Factory.newInstance(), colorMap);
}

/**
* TEST ONLY
* @param clr awt Color
* @param map
*/
public XSSFColor(java.awt.Color clr) {
this();
public XSSFColor(java.awt.Color clr, IndexedColorMap map) {
this(map);
setColor(clr);
}

Expand Down
4 changes: 2 additions & 2 deletions src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFColorScaleFormatting.java
View file
Expand Up @@ -56,7 +56,7 @@ public XSSFColor[] getColors() {
CTColor[] ctcols = _scale.getColorArray();
XSSFColor[] c = new XSSFColor[ctcols.length];
for (int i=0; i<ctcols.length; i++) {
c[i] = new XSSFColor(ctcols[i], _indexedColorMap);
c[i] = XSSFColor.from(ctcols[i], _indexedColorMap);
}
return c;
}
Expand Down Expand Up @@ -89,7 +89,7 @@ public void setThresholds(ConditionalFormattingThreshold[] thresholds) {
* @return color from scale
*/
public XSSFColor createColor() {
return new XSSFColor(_scale.addNewColor(), _indexedColorMap);
return XSSFColor.from(_scale.addNewColor(), _indexedColorMap);
}
public XSSFConditionalFormattingThreshold createThreshold() {
return new XSSFConditionalFormattingThreshold(_scale.addNewCfvo());
Expand Down
2 changes: 1 addition & 1 deletion src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFCreationHelper.java
View file
Expand Up @@ -54,7 +54,7 @@ public XSSFDataFormat createDataFormat() {

@Override
public XSSFColor createExtendedColor() {
return new XSSFColor(CTColor.Factory.newInstance(), workbook.getStylesSource().getIndexedColors());
return XSSFColor.from(CTColor.Factory.newInstance(), workbook.getStylesSource().getIndexedColors());
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFDataBarFormatting.java
View file
Expand Up @@ -66,7 +66,7 @@ public void setWidthMax(int width) {
}

public XSSFColor getColor() {
return new XSSFColor(_databar.getColor(), _colorMap);
return XSSFColor.from(_databar.getColor(), _colorMap);
}
public void setColor(Color color) {
_databar.setColor( ((XSSFColor)color).getCTColor() );
Expand Down
2 changes: 1 addition & 1 deletion src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFFont.java
View file
Expand Up @@ -157,7 +157,7 @@ public short getColor() {
public XSSFColor getXSSFColor() {
CTColor ctColor = _ctFont.sizeOfColorArray() == 0 ? null : _ctFont.getColorArray(0);
if(ctColor != null) {
XSSFColor color = new XSSFColor(ctColor, _indexedColorMap);
XSSFColor color = XSSFColor.from(ctColor, _indexedColorMap);
if(_themes != null) {
_themes.inheritFromThemeAsRequired(color);
}
Expand Down
2 changes: 1 addition & 1 deletion src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFFontFormatting.java
View file
Expand Up @@ -113,7 +113,7 @@ public void setFontColorIndex(short color){
public XSSFColor getFontColor() {
if(_font.sizeOfColorArray() == 0) return null;

return new XSSFColor(_font.getColorArray(0), _colorMap);
return XSSFColor.from(_font.getColorArray(0), _colorMap);
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFPatternFormatting.java
View file
Expand Up @@ -39,12 +39,12 @@ public class XSSFPatternFormatting implements PatternFormatting {

public XSSFColor getFillBackgroundColorColor() {
if(!_fill.isSetPatternFill()) return null;
return new XSSFColor(_fill.getPatternFill().getBgColor(), _colorMap);
return XSSFColor.from(_fill.getPatternFill().getBgColor(), _colorMap);
}
public XSSFColor getFillForegroundColorColor() {
if(!_fill.isSetPatternFill() || ! _fill.getPatternFill().isSetFgColor())
return null;
return new XSSFColor(_fill.getPatternFill().getFgColor(), _colorMap);
return XSSFColor.from(_fill.getPatternFill().getFgColor(), _colorMap);
}

public short getFillPattern() {
Expand Down
2 changes: 1 addition & 1 deletion src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java
View file
Expand Up @@ -4022,7 +4022,7 @@ public XSSFColor getTabColor() {
if (!pr.isSetTabColor()) {
return null;
}
return new XSSFColor(pr.getTabColor(), getWorkbook().getStylesSource().getIndexedColors());
return XSSFColor.from(pr.getTabColor(), getWorkbook().getStylesSource().getIndexedColors());
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/ooxml/java/org/apache/poi/xssf/usermodel/extensions/XSSFCellBorder.java
View file
Expand Up @@ -133,7 +133,7 @@ public XSSFColor getBorderColor(BorderSide side) {
CTBorderPr borderPr = getBorder(side);

if(borderPr != null && borderPr.isSetColor()) {
XSSFColor clr = new XSSFColor(borderPr.getColor(), _indexedColorMap);
XSSFColor clr = XSSFColor.from(borderPr.getColor(), _indexedColorMap);
if(_theme != null) {
_theme.inheritFromThemeAsRequired(clr);
}
Expand Down
16 changes: 12 additions & 4 deletions src/ooxml/java/org/apache/poi/xssf/usermodel/extensions/XSSFCellFill.java
View file
Expand Up @@ -60,7 +60,7 @@ public XSSFColor getFillBackgroundColor() {
if (ptrn == null) return null;

CTColor ctColor = ptrn.getBgColor();
return ctColor == null ? null : new XSSFColor(ctColor, _indexedColorMap);
return XSSFColor.from(ctColor, _indexedColorMap);
}

/**
Expand All @@ -81,7 +81,11 @@ public void setFillBackgroundColor(int index) {
*/
public void setFillBackgroundColor(XSSFColor color) {
CTPatternFill ptrn = ensureCTPatternFill();
ptrn.setBgColor(color.getCTColor());
if (color == null) {
ptrn.unsetBgColor();
} else {
ptrn.setBgColor(color.getCTColor());
}
}

/**
Expand All @@ -94,7 +98,7 @@ public XSSFColor getFillForegroundColor() {
if (ptrn == null) return null;

CTColor ctColor = ptrn.getFgColor();
return ctColor == null ? null : new XSSFColor(ctColor, _indexedColorMap);
return XSSFColor.from(ctColor, _indexedColorMap);
}

/**
Expand All @@ -115,7 +119,11 @@ public void setFillForegroundColor(int index) {
*/
public void setFillForegroundColor(XSSFColor color) {
CTPatternFill ptrn = ensureCTPatternFill();
ptrn.setFgColor(color.getCTColor());
if (color == null) {
ptrn.unsetFgColor();
} else {
ptrn.setFgColor(color.getCTColor());
}
}

/**
Expand Down

0 comments on commit 11e60eb

Please sign in to comment.