Skip to content

Commit 5c63643

Browse files
committed
fix: prevent table corruption when adding column after deleted var-length column
mutateAddColumn() wrote the current live var-column count instead of the historical max, corrupting the table definition and later causing raw ArrayIndexOutOfBoundsExceptions on read/write. Wrap remaining inconsistencies as JackcessException instead of leaking unchecked exceptions. Reported-by: @congjun-yang Closes #10
1 parent 863ae78 commit 5c63643

2 files changed

Lines changed: 80 additions & 2 deletions

File tree

src/main/java/io/github/spannm/jackcess/impl/TableImpl.java

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import java.lang.System.Logger;
2929
import java.lang.System.Logger.Level;
3030
import java.nio.BufferOverflowException;
31+
import java.nio.BufferUnderflowException;
3132
import java.nio.ByteBuffer;
3233
import java.nio.charset.Charset;
3334
import java.time.LocalDateTime;
@@ -765,7 +766,21 @@ private static Object getRowColumn(JetFormat format, ByteBuffer rowBuffer, Colum
765766
// cache "raw" row value. see note about caching above
766767
rowState.withRowCacheValue(column.getColumnIndex(), ColumnImpl.rawDataWrapper(columnData));
767768

768-
return rowState.handleRowError(column, columnData, _ex);
769+
Exception rowError = _ex;
770+
if (_ex instanceof IndexOutOfBoundsException || _ex instanceof NegativeArraySizeException
771+
|| _ex instanceof BufferUnderflowException || _ex instanceof BufferOverflowException) {
772+
// these particular runtime exceptions typically indicate corrupted or
773+
// inconsistent table metadata (e.g. bad column offsets) rather than a
774+
// normal, expected read failure. wrap them in a proper (checked)
775+
// IOException so that callers relying on the declared "throws
776+
// IOException" contract of the read methods do not have to also
777+
// anticipate an arbitrary unchecked exception escaping
778+
rowError = new JackcessException(
779+
"Likely data corruption encountered reading column " + column.getName()
780+
+ " of table " + column.getTable().getName(), _ex);
781+
}
782+
783+
return rowState.handleRowError(column, columnData, rowError);
769784
}
770785
}
771786

@@ -1077,7 +1092,7 @@ protected ColumnImpl mutateAddColumn(TableUpdater mutator) throws IOException {
10771092
// update various bits of the table def
10781093
ByteUtil.forward(tableBuffer, 29);
10791094
tableBuffer.putShort((short) (_maxColumnCount + 1));
1080-
short varColCount = (short) (_varColumns.size() + (isVarCol ? 1 : 0));
1095+
short varColCount = (short) (_maxVarColumnCount + (isVarCol ? 1 : 0));
10811096
tableBuffer.putShort(varColCount);
10821097
tableBuffer.putShort((short) (_columns.size() + 1));
10831098

@@ -2546,6 +2561,17 @@ private ByteBuffer createRow(Object[] rowArray, ByteBuffer buffer, int minRowSiz
25462561
short[] varColumnOffsets = new short[_maxVarColumnCount];
25472562
int varColumnOffsetsIndex = 0;
25482563
for (ColumnImpl varCol : _varColumns) {
2564+
if (varCol.getVarLenTableIndex() >= varColumnOffsets.length) {
2565+
// this indicates an inconsistency between the number of variable
2566+
// length columns recorded in the table definition and the actual
2567+
// columns present (e.g. a corrupted table definition); fail with a
2568+
// clear, checked exception instead of an opaque
2569+
// ArrayIndexOutOfBoundsException
2570+
throw new JackcessException(withErrorContext(
2571+
"Table definition is inconsistent: variable length column " + varCol.getName()
2572+
+ " has an out of range column offset index; the table definition may be corrupted"));
2573+
}
2574+
25492575
short offset = (short) buffer.position();
25502576
Object rowValue = varCol.getRowValue(rowArray);
25512577
if (rowValue != null) {

src/test/java/io/github/spannm/jackcess/DatabaseTest.java

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -304,6 +304,58 @@ void testReadWithDeletedCols(TestDb testDb) throws IOException {
304304
}
305305
}
306306

307+
@ParameterizedTest(name = "[{index}] {0}")
308+
@TestDbSource(DEL_COL)
309+
void testAddColumnAfterDeletedVarLenColumns(TestDb testDb) throws IOException {
310+
// regression test for GH issue #10: adding a column to a table which
311+
// previously had variable length columns deleted from it must not corrupt
312+
// the table definition's variable column count. the corruption is only
313+
// persisted to (and later observable from) disk, so the db must be closed
314+
// and reopened to actually exercise the bug
315+
File dbFile;
316+
try (Database db = testDb.openCopy()) {
317+
dbFile = db.getFile();
318+
Table table = db.getTable("Table1");
319+
DatabaseBuilder.newColumn("newCol", DataType.BOOLEAN).addToTable(table);
320+
}
321+
322+
Map<String, Object> newRow = new LinkedHashMap<>(Map.of(
323+
"id", 9, "id2", 9, "data", "baz", "data2", "baz2", "newCol", Boolean.TRUE));
324+
325+
try (Database db = DatabaseBuilder.open(dbFile)) {
326+
Table table = db.getTable("Table1");
327+
table.addRow(newRow.get("id"), newRow.get("id2"), newRow.get("data"), newRow.get("data2"), newRow.get("newCol"));
328+
}
329+
330+
Map<String, Object> expectedRow0 = new LinkedHashMap<>(Map.of(
331+
"id", 0, "id2", 2, "data", "foo", "data2", "foo2"));
332+
expectedRow0.put("newCol", Boolean.FALSE);
333+
334+
Map<String, Object> expectedRow1 = new LinkedHashMap<>(Map.of(
335+
"id", 3, "id2", 5, "data", "bar", "data2", "bar2"));
336+
expectedRow1.put("newCol", Boolean.FALSE);
337+
338+
try (Database db = DatabaseBuilder.open(dbFile)) {
339+
Table table = db.getTable("Table1");
340+
341+
int rowNum = 0;
342+
Map<String, Object> row = null;
343+
while ((row = table.getNextRow()) != null) {
344+
if (rowNum == 0) {
345+
assertEquals(expectedRow0, row);
346+
} else if (rowNum == 1) {
347+
assertEquals(expectedRow1, row);
348+
} else if (rowNum == 2) {
349+
assertEquals(newRow, row);
350+
} else {
351+
fail("should only have 3 rows");
352+
}
353+
rowNum++;
354+
}
355+
assertEquals(3, rowNum);
356+
}
357+
}
358+
307359
@ParameterizedTest(name = "[{index}] {0}")
308360
@FileFormatSource
309361
void testCurrency(FileFormat fileFormat) throws IOException {

0 commit comments

Comments
 (0)