From 8827c07cdbd44a5863a234bda59483186ced201d Mon Sep 17 00:00:00 2001 From: Nick Burch Date: Fri, 15 Feb 2008 11:53:25 +0000 Subject: [PATCH] Fix for bug #44413 from Josh - Fix for circular references in INDEX, OFFSET, VLOOKUP formulas, where a cell is actually allowed to reference itself git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@628029 13f79535-47bb-0310-9956-ffa450edef68 --- src/documentation/content/xdocs/changes.xml | 1 + src/documentation/content/xdocs/status.xml | 1 + .../usermodel/EvaluationCycleDetector.java | 150 ++++++++++++++++++ .../EvaluationCycleDetectorManager.java | 46 ++++++ .../hssf/usermodel/HSSFFormulaEvaluator.java | 26 +-- .../formula/eval/TestCircularReferences.java | 125 +++++++++++++++ 6 files changed, 340 insertions(+), 9 deletions(-) create mode 100755 src/scratchpad/src/org/apache/poi/hssf/usermodel/EvaluationCycleDetector.java create mode 100755 src/scratchpad/src/org/apache/poi/hssf/usermodel/EvaluationCycleDetectorManager.java create mode 100755 src/scratchpad/testcases/org/apache/poi/hssf/record/formula/eval/TestCircularReferences.java diff --git a/src/documentation/content/xdocs/changes.xml b/src/documentation/content/xdocs/changes.xml index d8bbb40a1..1c91e62f8 100644 --- a/src/documentation/content/xdocs/changes.xml +++ b/src/documentation/content/xdocs/changes.xml @@ -36,6 +36,7 @@ + 44413 - Fix for circular references in INDEX, OFFSET, VLOOKUP formulas, where a cell is actually allowed to reference itself 44403 - Fix for Mid function handling its arguments wrong 44364 - Support for Match, NA and SumProduct functions, as well as initial function error support 44375 - Cope with a broken dictionary in Document Summary Information stream. RuntimeExceptions that occured when trying to read bogus data are now caught. Dictionary entries up to but not including the bogus one are preserved, the rest is ignored. diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml index 73e2f6852..67d1ddd4c 100644 --- a/src/documentation/content/xdocs/status.xml +++ b/src/documentation/content/xdocs/status.xml @@ -33,6 +33,7 @@ + 44413 - Fix for circular references in INDEX, OFFSET, VLOOKUP formulas, where a cell is actually allowed to reference itself 44403 - Fix for Mid function handling its arguments wrong 44364 - Support for Match, NA and SumProduct functions, as well as initial function error support 44375 - Cope with a broken dictionary in Document Summary Information stream. RuntimeExceptions that occured when trying to read bogus data are now caught. Dictionary entries up to but not including the bogus one are preserved, the rest is ignored. diff --git a/src/scratchpad/src/org/apache/poi/hssf/usermodel/EvaluationCycleDetector.java b/src/scratchpad/src/org/apache/poi/hssf/usermodel/EvaluationCycleDetector.java new file mode 100755 index 000000000..90f5807ff --- /dev/null +++ b/src/scratchpad/src/org/apache/poi/hssf/usermodel/EvaluationCycleDetector.java @@ -0,0 +1,150 @@ +/* ==================================================================== + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +==================================================================== */ + +package org.apache.poi.hssf.usermodel; + +import java.util.ArrayList; +import java.util.List; + +/** + * Instances of this class keep track of multiple dependent cell evaluations due + * to recursive calls to HSSFFormulaEvaluator.internalEvaluate(). + * The main purpose of this class is to detect an attempt to evaluate a cell + * that is already being evaluated. In other words, it detects circular + * references in spreadsheet formulas. + * + * @author Josh Micich + */ +final class EvaluationCycleDetector { + + /** + * Stores the parameters that identify the evaluation of one cell.
+ */ + private static final class CellEvaluationFrame { + + private final HSSFWorkbook _workbook; + private final HSSFSheet _sheet; + private final int _srcRowNum; + private final int _srcColNum; + + public CellEvaluationFrame(HSSFWorkbook workbook, HSSFSheet sheet, int srcRowNum, int srcColNum) { + if (workbook == null) { + throw new IllegalArgumentException("workbook must not be null"); + } + if (sheet == null) { + throw new IllegalArgumentException("sheet must not be null"); + } + _workbook = workbook; + _sheet = sheet; + _srcRowNum = srcRowNum; + _srcColNum = srcColNum; + } + + public boolean equals(Object obj) { + CellEvaluationFrame other = (CellEvaluationFrame) obj; + if (_workbook != other._workbook) { + return false; + } + if (_sheet != other._sheet) { + return false; + } + if (_srcRowNum != other._srcRowNum) { + return false; + } + if (_srcColNum != other._srcColNum) { + return false; + } + return true; + } + + /** + * @return human readable string for debug purposes + */ + public String formatAsString() { + return "R=" + _srcRowNum + " C=" + _srcColNum + " ShIx=" + _workbook.getSheetIndex(_sheet); + } + + public String toString() { + StringBuffer sb = new StringBuffer(64); + sb.append(getClass().getName()).append(" ["); + sb.append(formatAsString()); + sb.append("]"); + return sb.toString(); + } + } + + private final List _evaluationFrames; + + public EvaluationCycleDetector() { + _evaluationFrames = new ArrayList(); + } + + /** + * Notifies this evaluation tracker that evaluation of the specified cell is + * about to start.
+ * + * In the case of a true return code, the caller should + * continue evaluation of the specified cell, and also be sure to call + * endEvaluate() when complete.
+ * + * In the case of a false return code, the caller should + * return an evaluation result of + * ErrorEval.CIRCULAR_REF_ERROR, and not call endEvaluate(). + *
+ * @return true if the specified cell has not been visited yet in the current + * evaluation. false if the specified cell is already being evaluated. + */ + public boolean startEvaluate(HSSFWorkbook workbook, HSSFSheet sheet, int srcRowNum, int srcColNum) { + CellEvaluationFrame cef = new CellEvaluationFrame(workbook, sheet, srcRowNum, srcColNum); + if (_evaluationFrames.contains(cef)) { + return false; + } + _evaluationFrames.add(cef); + return true; + } + + /** + * Notifies this evaluation tracker that the evaluation of the specified + * cell is complete.

+ * + * Every successful call to startEvaluate must be followed by a + * call to endEvaluate (recommended in a finally block) to enable + * proper tracking of which cells are being evaluated at any point in time.

+ * + * Assuming a well behaved client, parameters to this method would not be + * required. However, they have been included to assert correct behaviour, + * and form more meaningful error messages. + */ + public void endEvaluate(HSSFWorkbook workbook, HSSFSheet sheet, int srcRowNum, int srcColNum) { + int nFrames = _evaluationFrames.size(); + if (nFrames < 1) { + throw new IllegalStateException("Call to endEvaluate without matching call to startEvaluate"); + } + + nFrames--; + CellEvaluationFrame cefExpected = (CellEvaluationFrame) _evaluationFrames.get(nFrames); + CellEvaluationFrame cefActual = new CellEvaluationFrame(workbook, sheet, srcRowNum, srcColNum); + if (!cefActual.equals(cefExpected)) { + throw new RuntimeException("Wrong cell specified. " + + "Corresponding startEvaluate() call was for cell {" + + cefExpected.formatAsString() + "} this endEvaluate() call is for cell {" + + cefActual.formatAsString() + "}"); + } + // else - no problems so pop current frame + _evaluationFrames.remove(nFrames); + } +} diff --git a/src/scratchpad/src/org/apache/poi/hssf/usermodel/EvaluationCycleDetectorManager.java b/src/scratchpad/src/org/apache/poi/hssf/usermodel/EvaluationCycleDetectorManager.java new file mode 100755 index 000000000..a06cd201e --- /dev/null +++ b/src/scratchpad/src/org/apache/poi/hssf/usermodel/EvaluationCycleDetectorManager.java @@ -0,0 +1,46 @@ +/* ==================================================================== + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +==================================================================== */ + +package org.apache.poi.hssf.usermodel; + +/** + * This class makes an EvaluationCycleDetector instance available to + * each thread via a ThreadLocal in order to avoid adding a parameter + * to a few protected methods within HSSFFormulaEvaluator. + * + * @author Josh Micich + */ +final class EvaluationCycleDetectorManager { + + ThreadLocal tl = null; + private static ThreadLocal _tlEvaluationTracker = new ThreadLocal() { + protected synchronized Object initialValue() { + return new EvaluationCycleDetector(); + } + }; + + /** + * @return + */ + public static EvaluationCycleDetector getTracker() { + return (EvaluationCycleDetector) _tlEvaluationTracker.get(); + } + + private EvaluationCycleDetectorManager() { + // no instances of this class + } +} diff --git a/src/scratchpad/src/org/apache/poi/hssf/usermodel/HSSFFormulaEvaluator.java b/src/scratchpad/src/org/apache/poi/hssf/usermodel/HSSFFormulaEvaluator.java index 70b78dfae..fb0c980e8 100644 --- a/src/scratchpad/src/org/apache/poi/hssf/usermodel/HSSFFormulaEvaluator.java +++ b/src/scratchpad/src/org/apache/poi/hssf/usermodel/HSSFFormulaEvaluator.java @@ -96,8 +96,6 @@ import org.apache.poi.hssf.usermodel.HSSFSheet; /** * @author Amol S. Deshmukh < amolweb at ya hoo dot com > * - * Limitations: Unfortunately, cyclic references will cause stackoverflow - * exception */ public class HSSFFormulaEvaluator { @@ -353,16 +351,26 @@ public class HSSFFormulaEvaluator { * Dev. Note: Internal evaluate must be passed only a formula cell * else a runtime exception will be thrown somewhere inside the method. * (Hence this is a private method.) - * - * @param srcCell - * @param srcRow - * @param sheet - * @param workbook */ - protected static ValueEval internalEvaluate(HSSFCell srcCell, HSSFRow srcRow, HSSFSheet sheet, HSSFWorkbook workbook) { + private static ValueEval internalEvaluate(HSSFCell srcCell, HSSFRow srcRow, HSSFSheet sheet, HSSFWorkbook workbook) { int srcRowNum = srcRow.getRowNum(); short srcColNum = srcCell.getCellNum(); - FormulaParser parser = new FormulaParser(srcCell.getCellFormula(), workbook.getWorkbook()); + + + EvaluationCycleDetector tracker = EvaluationCycleDetectorManager.getTracker(); + + if(!tracker.startEvaluate(workbook, sheet, srcRowNum, srcColNum)) { + return ErrorEval.CIRCULAR_REF_ERROR; + } + try { + return evaluateCell(workbook, sheet, srcRowNum, srcColNum, srcCell.getCellFormula()); + } finally { + tracker.endEvaluate(workbook, sheet, srcRowNum, srcColNum); + } + } + private static ValueEval evaluateCell(HSSFWorkbook workbook, HSSFSheet sheet, + int srcRowNum, short srcColNum, String cellFormulaText) { + FormulaParser parser = new FormulaParser(cellFormulaText, workbook.getWorkbook()); parser.parse(); Ptg[] ptgs = parser.getRPNPtg(); // -- parsing over -- diff --git a/src/scratchpad/testcases/org/apache/poi/hssf/record/formula/eval/TestCircularReferences.java b/src/scratchpad/testcases/org/apache/poi/hssf/record/formula/eval/TestCircularReferences.java new file mode 100755 index 000000000..72db658f7 --- /dev/null +++ b/src/scratchpad/testcases/org/apache/poi/hssf/record/formula/eval/TestCircularReferences.java @@ -0,0 +1,125 @@ +/* ==================================================================== + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +==================================================================== */ + +package org.apache.poi.hssf.record.formula.eval; + +import junit.framework.AssertionFailedError; +import junit.framework.TestCase; + +import org.apache.poi.hssf.usermodel.HSSFCell; +import org.apache.poi.hssf.usermodel.HSSFFormulaEvaluator; +import org.apache.poi.hssf.usermodel.HSSFRow; +import org.apache.poi.hssf.usermodel.HSSFSheet; +import org.apache.poi.hssf.usermodel.HSSFWorkbook; +import org.apache.poi.hssf.usermodel.HSSFFormulaEvaluator.CellValue; +/** + * Tests HSSFFormulaEvaluator for its handling of cell formula circular references. + * + * @author Josh Micich + */ +public final class TestCircularReferences extends TestCase { + /** + * Translates StackOverflowError into AssertionFailedError + */ + private static CellValue evaluateWithCycles(HSSFWorkbook wb, HSSFSheet sheet, HSSFRow row, HSSFCell testCell) + throws AssertionFailedError { + HSSFFormulaEvaluator evaluator = new HSSFFormulaEvaluator(sheet, wb); + evaluator.setCurrentRow(row); + try { + return evaluator.evaluate(testCell); + } catch (StackOverflowError e) { + throw new AssertionFailedError( "circular reference caused stack overflow error"); + } + } + /** + * Makes sure that the specified evaluated cell value represents a circular reference error. + */ + private static void confirmCycleErrorCode(CellValue cellValue) { + assertTrue(cellValue.getCellType() == HSSFCell.CELL_TYPE_ERROR); + assertEquals(ErrorEval.CIRCULAR_REF_ERROR.getErrorCode(), cellValue.getErrorValue()); + } + + + /** + * ASF Bugzilla Bug 44413 + * "INDEX() formula cannot contain its own location in the data array range" + */ + public void testIndexFormula() { + + HSSFWorkbook wb = new HSSFWorkbook(); + HSSFSheet sheet = wb.createSheet("Sheet1"); + + short colB = 1; + sheet.createRow(0).createCell(colB).setCellValue(1); + sheet.createRow(1).createCell(colB).setCellValue(2); + sheet.createRow(2).createCell(colB).setCellValue(3); + HSSFRow row4 = sheet.createRow(3); + HSSFCell testCell = row4.createCell((short)0); + // This formula should evaluate to the contents of B2, + testCell.setCellFormula("INDEX(A1:B4,2,2)"); + // However the range A1:B4 also includes the current cell A4. If the other parameters + // were 4 and 1, this would represent a circular reference. Since POI 'fully' evaluates + // arguments before invoking operators, POI must handle such potential cycles gracefully. + + + CellValue cellValue = evaluateWithCycles(wb, sheet, row4, testCell); + + assertTrue(cellValue.getCellType() == HSSFCell.CELL_TYPE_NUMERIC); + assertEquals(2, cellValue.getNumberValue(), 0); + } + + /** + * Cell A1 has formula "=A1" + */ + public void testSimpleCircularReference() { + + HSSFWorkbook wb = new HSSFWorkbook(); + HSSFSheet sheet = wb.createSheet("Sheet1"); + + HSSFRow row = sheet.createRow(0); + HSSFCell testCell = row.createCell((short)0); + testCell.setCellFormula("A1"); + + HSSFFormulaEvaluator evaluator = new HSSFFormulaEvaluator(sheet, wb); + evaluator.setCurrentRow(row); + CellValue cellValue = evaluateWithCycles(wb, sheet, row, testCell); + + confirmCycleErrorCode(cellValue); + } + + /** + * A1=B1, B1=C1, C1=D1, D1=A1 + */ + public void testMultiLevelCircularReference() { + + HSSFWorkbook wb = new HSSFWorkbook(); + HSSFSheet sheet = wb.createSheet("Sheet1"); + + HSSFRow row = sheet.createRow(0); + row.createCell((short)0).setCellFormula("B1"); + row.createCell((short)1).setCellFormula("C1"); + row.createCell((short)2).setCellFormula("D1"); + HSSFCell testCell = row.createCell((short)3); + testCell.setCellFormula("A1"); + + HSSFFormulaEvaluator evaluator = new HSSFFormulaEvaluator(sheet, wb); + evaluator.setCurrentRow(row); + CellValue cellValue = evaluateWithCycles(wb, sheet, row, testCell); + + confirmCycleErrorCode(cellValue); + } +}