Move parsing of SAS column name subheader to first pass

[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.