Read all column info for empty SAS data files

[hpoettker]

This PR fixes a problem that I've observed with empty SAS data files that have a very high number (literally thousands) of columns.

An empty SAS data file can extend over multiple pages if the number of columns is high enough. When for such a file a column name header is on the second page or beyond, this header is currently not being read. The root cause is an early exit for the second pass that exits when the number of expected rows has been reached, which is always after the first page for empty data files.

In ReadStat the bug manifests itself in blank column names. Downstream in pyreadstat the bug produces errors due to column names with value None which pyreadstat isn't (and shouldn't be) prepared to handle.

[hpoettker]

The change can also be validated by the SAS files that ReadStat produces itself. So a test like this should in theory work: hpoettker@e14ea64

In practice, it turns out that the chopped column information is a bigger problem for the test framework than for the library itself. A lot of assertions on columns are tested before one can be sure that all column data has been loaded.

The suspicion falls on the test framework as the dumped buffer from the failing test can be read correctly with the command line utility readstat from this PR.

I'll have a deeper look but it might take some time.

[hpoettker]

I've found a different and perhaps even more elegant solution for the problem.

The root cause of the issue is that the SAS column name subheaders are currently not always read. One particular case are multi-page SAS data files without any rows that have column name subheaders beyond the first page.

The new proposal is to move the parsing of the column name subheaders from the second to the first pass. This is possible as parsing the column name subheaders does not depend on the column text subheaders having been parsed before.

This also resolves the issue in the testing framework as it does its assertions in the second pass and can now depend on the column name subheaders to be parsed.

The added test fails on the current dev branch and succeeds with the proposed change.

[evanmiller]

I asked Fable 5 to review, would appreciate an update based on the findings

Code Review: PR #365 — SAS7BDAT moved-rows / REFERENCE-pointer support

Review of WizardMac/ReadStat#365, ranked most severe first. The top items were verified directly against the code.

Memory safety

1. Out-of-bounds heap read in the NONE_MOVED branch

src/sas/readstat_sas7bdat_read.c:1079

sas7bdat_validate_subheader_pointer only enforces the offset + subheader_signature_size > page_size bound when compression is SAS_COMPRESSION_NONE (0x00). The new NONE_MOVED (0x02) path passes validation with offset = page_size - 2, len = 2, then calls sas7bdat_parse_subheader_type(page + offset), which reads 8 bytes — 6 past the end of the malloc'd page buffer. The len != row_length check that would catch it comes at line 1084, after the read.

Failure scenario: A crafted file has a REFERENCE (0x03) pointer routing to a target page whose subheader pointer has compression == SAS_COMPRESSION_NONE_MOVED, offset = page_size - 2, len = 2. Validation passes; the 8-byte read at page + offset runs off the end of the heap allocation.

2. Read failure on the moved-to page goes undetected

src/sas/readstat_sas7bdat_read.c:1045

io->read(page, page_size, ...) < page_size compares an ssize_t against a uint64_t. A handler returning -1 promotes to UINT64_MAX, so the error check is skipped and parsing runs over uninitialized heap memory instead of returning READSTAT_ERROR_READ.

Failure scenario: A custom readstat_io_t read handler returns -1 (transient/network I/O error) while fetching the moved-to page. Subheader parsing runs over the freshly malloc'd, non-zeroed buffer, yielding garbage values (use-of-uninitialized-value under MSan).

Correctness

3. Duplicate rows plus silent truncation

src/sas/readstat_sas7bdat_read.c:1141

Nothing tracks which moved subheaders a REFERENCE pointer has already consumed. Two REFERENCE pointers resolving to the same (page_index, subheader_index) emit the row twice; parsed_row_count then hits row_limit early and the pass2 loop breaks before reading later pages — a duplicated row and silently dropped trailing rows, all returned as READSTAT_OK. Previously 0x03 was a hard READSTAT_ERROR_UNSUPPORTED_COMPRESSION.

4. Loud errors became silent row loss (SAS_COMPRESSION_MYSTERY)

src/sas/readstat_sas7bdat_read.c:981, 1179 and src/sas/readstat_sas.h:131

Compression code 0x0d previously raised READSTAT_ERROR_UNSUPPORTED_COMPRESSION at the offending page; it is now silently voided in both passes with no recovery path. If a 0x0d subheader actually holds row data not targeted by any REFERENCE pointer, the row either vanishes with READSTAT_OK or surfaces only as a vague end-of-parse ROW_COUNT_MISMATCH (and that final check is skipped entirely when no value handler is registered).

A binary scan of both new test resources found no 0x0d compression byte (compression_type_2: {0x0, 0x1, 0x2, 0x3, 0x4}; compression_type_6: {0x0, 0x1, 0x3, 0x4, 0x6}), so the code is speculative. Drop it, or keep it only with a comment citing a real file that produces it.

5. Stale file position after a moved-row seek

src/sas/readstat_sas7bdat_read.c:1359–1382

sas7bdat_update_progress runs before the requires_page_seek restore, so after a REFERENCE pointer resolves to a distant page, progress is computed from the moved-to page's offset and jumps non-monotonically (a consumer that treats regression as an error would abort the parse; the final update can report < 100% on success). Separately, the "Error parsing page" diagnostic computes its byte range from io->seek(0, SEEK_CUR), so errors later on the same page report the wrong bytes.

Suggested fix: Restore the position inside sas7bdat_parse_moved_row itself — save io->seek(0, SEEK_CUR) on entry, seek back before return. That fixes both symptoms, deletes the requires_page_seek ctx field and the deferred re-seek block, and removes one of two duplicated "Failed to seek" error snippets.

6. Uncompressed files reported as row-compressed

src/sas/readstat_sas7bdat_read.c:1074

parse_moved_row always calls sas7bdat_submit_columns_if_needed(ctx, 1), so if the first row-bearing item hit in pass2 is a REFERENCE pointer, metadata.compression is set to READSTAT_COMPRESS_ROWS even for an uncompressed file (the PR's own compression_type_2.sas7bdat is tagged COMP_NONE). Lower confidence — the sibling path at line 1155 passes the same flag for the uncompressed shape, so this may be an intended convention.

7. Off-by-one over-strict bound

src/sas/readstat_sas7bdat_read.c:1061

shp_offset + subheader_pointer_size >= page_size rejects a subheader-pointer array that ends exactly at the page boundary, which the equivalent checks in pass1/pass2 (lines 959, 1128: ... > page_size) accept. Should be >. Not a memory-safety issue, but it can spuriously fail legitimate files with an unexplained READSTAT_ERROR_READ.

Tests and quality

8. The new tests skip the column that matters

src/test/test_read.c:268

The new skip_value_comparison escape hatch exempts payload_to_update from value comparison in both new fixtures — the very column whose update caused the row move, i.e. the bytes the moved-row code reconstructs. A regression in the NONE_MOVED or ROW_MOVED decompression path that returns stale, truncated, or garbage payload for the moved row would still pass, because the fixed-offset id/category fields survive corruption. Encoding the expected strings (or a short known payload) would restore the coverage.

9. I/O and allocation amplification

src/sas/readstat_sas7bdat_read.c:1041

Every REFERENCE pointer performs a fresh seek + readstat_malloc(page_size) + full page read + free, with no caching and no check for the target being the page already loaded in ctx->page. A dataset with thousands of updated rows re-reads the same few target pages thousands of times (hundreds of MB of redundant I/O at large page sizes). A one-entry page cache in ctx, plus a shortcut when page_index equals the current loop page, would reduce this to a handful of reads and one allocation.

10. Three hand-synchronized compression-code lists

src/sas/readstat_sas7bdat_read.c:981 (pass1 allowlist), 1179 (pass2 skip-list), and the parse_moved_row branch

The recognized/void compression codes are maintained as three separate OR-chains the compiler cannot keep in sync — this PR itself had to grow all three in lockstep to add three codes. A code added to pass2 but not pass1 dies before any data is read; added to pass1 but not pass2 it aborts mid-file after rows were delivered. A single classifier helper (e.g. sas7bdat_compression_is_row_marker) would remove the hazard.

Relatedly, the two new src/test/test_list.h entries (lines 2389–2423, 2426–2460) are 35-line blocks identical except resource_name/test_formats and share the same .label ("SAS file with moved rows"), so a failure can't be attributed to the right fixture. Factor the .columns block into a shared define and give distinct labels.

What the PR gets right

The bounds-checking that was added is otherwise solid: parse_moved_row correctly validates page_index >= ctx->page_count (catching the offset-0 underflow), subheader_index >= subheader_count, the single-pointer bounds check, and full sas7bdat_validate_subheader_pointer coverage for the cases it was designed for. Skipping REFERENCE validation in pass1 is safe because pass1 never dereferences those pointers, and double-emission through the normal sequential path is prevented since moved subheaders (0x02/0x06) are voided when their host page is reached in order.

Verdict

Findings #1 and #2 are blocking (memory safety). #3–#5 should be fixed before merge — they are exactly the corrupt-file territory this parser gets fuzzed on. The rest are quality improvements at the author's discretion.

[hpoettker]

Thanks for the review on #365! It's posted here in the wrong PR but it's of course clear from the title of the comment which PR it refers to.

If you could post a Fable review for this PR as well that would be very much appreciated. This PR addresses the only remaining issue that I've found while reading SAS data files in my personal use case.

There are some more issues in reading SAS catalog files but that's of lower priority to me at the moment.

[evanmiller]

It's posted here in the wrong PR

Human error :-)

Here is a Fable 5.1 review of this PR, hot off the presses....

Code Review: PR #384 — Move parsing of SAS column name subheader to first pass

Review of WizardMac/ReadStat#384 (head a649cb2, base da9fcaa), ranked most severe first. Every finding below was verified by building the base branch and the PR branch and running test_readstat, plus a small readback harness linked against each build.

Summary

The PR moves SAS_SUBHEADER_TYPE_COLUMN_NAME parsing from pass 2 to pass 1 so that column-name subheaders on page 2+ of a 0-row file are read, and adds an 80-column empty-file test. The reader change is safe and the test does fail on dev and pass with the PR. But the change fixes only the name symptom of a broader bug, and the new test is green only because the harness happens not to check the metadata that is still lost. A different one-line change fixes the whole problem; details in #1.

Correctness

1. Column attributes, formats and labels are still dropped for 0-row multi-page files

src/sas/readstat_sas7bdat_read.c:1274 (unchanged by the PR)

The root cause the PR description identifies — pass 2 breaks out after the first page because parsed_row_count == row_limit is already 0 == 0 — is left in place. Column names are rescued by parsing them in pass 1, but COLUMN_ATTRS (type, width, offset) and the per-column COLUMN_FORMAT subheaders (format and label refs) are still parsed only in pass 2, so anything of theirs beyond page 0 is never read.

The PR's own test file demonstrates it. Page layout of the 64-bit file the test writes (page size 4096):

page	subheaders
0	ROW_SIZE, COL_SIZE, COL_TEXT
1	COL_NAME, COL_ATTRS, COL_FORMAT ×23
2	COL_FORMAT ×46
3	COL_FORMAT ×11

Reading that file back through the PR build, every one of the 80 variables comes out as type=0 (STRING) storage_width=0 format="" label="". The writer gave them width 8. Changing column 80 to READSTAT_TYPE_DOUBLE still reads back as STRING with width 0. Adding .label/.format to the columns makes the PR's test fail with Column labels ... Received: (null) — which is exactly the failure the author hit with the earlier $CHAR8 version of this test and attributed to the test framework. It is not a framework problem; it is the same bug, still present.

For the pyreadstat use case that motivated the PR, this means a wide empty file now gets correct column names but wrong dtypes (everything object/string), zero widths, and no labels or formats for every column whose metadata spilled past page 0.

Suggested fix. Skip the early exit when there are no rows to find:

-        if (ctx->parsed_row_count == ctx->row_limit)
+        if (ctx->row_limit > 0 && ctx->parsed_row_count == ctx->row_limit)
             break;

Verified on base + this change alone (without the pass-1 move): the PR's new test passes; a variant with labels, $CHAR8 formats and a DOUBLE column 80 passes; the readback shows width 8 on all columns, type=5 and the label on column 80; the full test_readstat suite (all three row_limit/row_offset arg sets) passes. Cost: a caller who sets row_offset past the last live row now reads every page instead of one, which is the correct thing to do anyway for a wide file. With this in place the pass-1 relocation is unnecessary; if it is kept for other reasons, see #2.

2. Pass 1 visits trailing AMD pages in reverse order, and column-name assignment is order-dependent

src/sas/readstat_sas7bdat_read.c:974 (new), :1197 (AMD loop), :319 (col_names_count += cmax)

sas7bdat_parse_column_name_subheader appends names by encounter order. Pass 2 walks pages strictly forward; pass 1 walks the leading META/MIX pages forward and then the trailing AMD run backward (for (i=page_count-1; i>last_examined_page_pass1; i--)). If a file ever carried COLUMN_NAME subheaders on two or more AMD pages, the PR would assign names to the wrong columns, silently. COLUMN_TEXT already has the same latent hazard, so this is not a new class of problem, and I have no evidence SAS emits name subheaders on AMD pages — but the PR extends the exposure from "text blob index" to "column identity", so it is worth a comment in the code if the relocation is kept. Note also that after the PR, pages pass 1 skips (anything between the first DATA page and the trailing AMD run) no longer get their column names parsed in pass 2 either; coverage is now identical to COLUMN_TEXT, which is consistent but is a change from before.

Tests and quality

3. RT_MAX_COLS 10 → 80 makes the static test table 7× larger

src/test/test_types.h:4

rt_test_file_t grows from 7,616 to 50,176 bytes, rt_test_group_t from 152 KB to 1,003,600 bytes, and _test_groups[] (21 groups × 20 slots) from ~3.2 MB to ~21 MB of initialised data. Measured: test_readstat goes from 3.5 MB to 21.5 MB (__DATA = 21,086,208). It links and runs fine, so this is a cost decision rather than a defect. The spill genuinely needs ~77+ 32-character names at a 4096-byte page, so a smaller count will not trigger the bug; the alternatives are generating this one fixture at runtime instead of statically, or accepting the size.

4. The test cannot catch the remaining bug because the harness never compares type or width

src/test/test_read.c:179 (handle_variable), src/test/test_list.h:679

handle_variable checks name, label, format, informat, display width and missing ranges, but not readstat_variable_get_type or readstat_variable_get_storage_width. That is why the new test stays green while every column reads back as a 0-width string. Two cheap improvements: give the 80 columns a .label and .format (which the PR's test currently omits and which fails today), and add a type comparison to handle_variable so a READSTAT_TYPE_DOUBLE column in the fixture is actually enforced.

5. Stale comment

src/sas/readstat_sas7bdat_read.c:948

/* First, extract column text */ above sas7bdat_parse_page_pass1 should mention column names if the relocation stays.

What the PR gets right

• The relocation itself is safe: sas7bdat_parse_column_name_subheader depends only on header-derived fields (u64, subheader_signature_size, bswap) and sas7bdat_realloc_col_info grows the array in either order relative to COLUMN_SIZE, so parsing names before column_count is known is fine.
• The two branches stay symmetric (parsed in pass 1, skipped in pass 2), so nothing is double-parsed.
• The new test is a real regression test for the name symptom: it fails on dev (Received: (null) for columns 1–80) and passes with the PR.
• The diagnosis in the PR description is correct; it is the fix that stops short.

Verdict

Not mergeable as-is. The reader change is harmless but incomplete, and the test as written locks in the incomplete behaviour. Recommended path: replace (or supplement) the pass-1 relocation with the row_limit > 0 guard on the pass-2 early exit (#1), extend the fixture with labels/formats and a numeric column, and ideally add a type check to the harness (#4). #3 is a maintainer's call on binary size.

[hpoettker]

Thanks a lot!

I've rebased on dev and addressed the review feedback.

I've applied the suggestion from point 1, which makes points 2 and 5 obsolete.

I addressed point 4 by adding labels and formats to the test columns. This also helped with point 3 as the metadata now spills over with only 20 columns instead of 80 columns. The size of the test_readstat binary is now below 6 MB.