Fix heap overflow uncompressing tables with variable-length string columns (#134) - #175
Open
cruzzil wants to merge 1 commit into
Open
Fix heap overflow uncompressing tables with variable-length string columns (#134)#175cruzzil wants to merge 1 commit into
cruzzil wants to merge 1 commit into
Conversation
…lumns
fits_uncompress_table sizes each column's slice of the decompression
buffer from the ZFORMn keyword, but compared the datatype code using
abs():
if (abs(coltype[ii]) == TBIT) {
} else if (abs(coltype[ii]) == TSTRING) {
width = 1;
} else if (coltype[ii] < 0) { /* variable length array */
ffbnfm returns a negative code for a variable length array, so a
variable length string column ('1PA') has coltype = -TSTRING and matched
the TSTRING case: its field was sized as 1 byte per row instead of the 8
bytes of a 'P' descriptor, and the extra 16 bytes per row that a VLA
column needs for the second set of descriptors were never added to the
buffer. fits_uncompress_table then gunzipped the descriptors into a
buffer that was far too small. The overflow is not caught, because the
destination is an interior pointer of the shared buffer and the realloc
callback handed to uncompress2mem_from_mem reallocs it, so funpack dies
with "realloc(): invalid next size" or corrupts the heap. The same
comparisons also mis-sized variable length bit columns.
Compare the datatype code without abs(), so that the tests match those
in fits_compress_table, and harden the decompression of an untrusted
table:
- pass NULL rather than realloc for the four in-place gunzip calls, so
a stream that expands beyond the space reserved for the column fails
with DATA_DECOMPRESSION_ERR instead of reallocating an interior
pointer, and bail out of the tile loop when it does;
- reject the table if the ZFORMn column widths do not add up to
ZNAXIS1, since the buffers are sized from one and indexed with the
other.
Fixes heasarc#134.
cruzzil
force-pushed
the
fix-134-vla-string-table
branch
from
August 14, 2026 15:06
643a241 to
ec1a183
Compare
esabol
suggested changes
Aug 14, 2026
| /* compressed table is corrupt. */ | ||
| if (*status > 0) { | ||
| ffpmsg("Error uncompressing a column of the compressed table"); | ||
| free(rm_buffer); free(cm_buffer); |
Contributor
There was a problem hiding this comment.
Style nitpick: Please put the two free()s on separate lines.
Otherwise, it all looks good time. Thank you!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #134:
funpackaborts withrealloc(): invalid next sizeon its ownfpack -tableoutput when the table has a1PAcolumn.Cause
fits_uncompress_tablesizes each column's slice of the decompression bufferfrom
ZFORMn, but compares the datatype code withabs():ffbnfmreturns a negative code for a variable-length array, so1PAhascoltype = -TSTRINGand took the string branch: 1 byte per row instead of an8-byte
Pdescriptor, and the extra 16 bytes per row for the second descriptorset were never added to
cm_size. The gunzipped blob needs(8 + 16) * rowspertilebytes and got1 * rowspertile.fits_compress_tablecompares without
abs(), so the compressed file is fine — only reading it backoverruns the buffer. The same comparisons also mis-size variable-length bit
columns.
Nothing catches the overrun because the destination is an interior pointer of
cm_bufferanduncompress2mem_from_memis givenrealloc, which thenreallocs a pointer that is not the start of a heap block.
Change
abs()calls, so the tests matchfits_compress_table.NULLinstead ofreallocfor the four in-place gunzip calls, so anover-long stream fails with
DATA_DECOMPRESSION_ERRinstead of reallocatingan interior pointer, and bail out of the tile loop when it does.
ZFORMnwidths do not sum toZNAXIS1; the buffersare sized from one and indexed with the other, so this would have caught the
bug above as a clean error.
Tests
New
test_compress_table_vla()intests/test_imcompress.c(this fork's unitsuite) round-trips a 600-row table with
1PA,1PE,1PJand8Acolumnsthrough
fits_compress_table/fits_uncompress_tableand checks every value.It aborts on
developand passes with this change.Also verified:
testprogoutput and.fitunchanged; the issue's reproducernow funpacks with all 3000 strings intact;
fpack -table/funpackround tripof
testprog.fitOK; valgrind and ASan clean on the new test and onfunpack.🤖 Generated with Claude Code