Read 64-bit multi-page SAS catalog files and skip format default values

[hpoettker]

Summary

This PR addresses three issues regarding the reading of SAS catalog files:

• it fixes the issue that XLSR records beyond the first page in 64 bit files are currently not found due to a wrong offset,
• it skips the "informats" in the file, which currently lead to parsing errors
• it skips the default values of custom formats also in the cases where the default value is not written last

I've tested the PR locally with 64 bit files on Linux and 32 bit files on Windows.

Offset for XLSR record on later pages

The current implementation always uses the page offset 16 when looking for XLSR records on later pages. But the offset 16 is only correct for 32-bit files. For 64-bit files, it is 32.

Currently, the XLSR records on later pages are missed in 64-bit files, which leads to any formats that are referenced from those later records to be missed.

Informats in catalog files

The current implementation is only prepared for formats, which map from numbers to either other numbers or strings. This leads to parsing errors when "informats" are encountered, which also map from strings.

The PR proposes to just skip informats, which can be identified in the catalog file by names starting with @.

I might contribute the code for the informat parsing in a follow-up PR. But I think that would require a discussion before-hand on how to integrate informats into the existing API. They are not value labels that should be used for outward presentation but rather mappings that should be used on input data to derive an internal representation. I don't know whether such a concept exists for SPSS or Stata files.

Default values in custom formats

The PR fixes an issue that occurs when reading custom formats from SAS catalog files whose default value is not saved as the last value in the physical catalog file.

Currently, ReadStat skips the default value correctly in a format created like this:

proc format;
  value myfmt
    1 = 'Yes'
    2 = 'No'
    other = 'Unknown';
run;

as SAS writes the labels in the order of encounter.

But for a format created like this:

proc format;
  value myfmt
    other = 'Unknown'
    1 = 'Yes'
    2 = 'No';
run;

which logically creates the same format, ReadStat currently reads the format to map

• from 1 to Unknown,
• from 2 to Yes,
• and skips the mapping to No.

It would be nice to also expose the default value through the API. But that would require a discussion on the API change before-hand as the current handlers for value label do not accept default values as far as I can tell. I don't know whether the concept of default value labels exists for SPSS or Stata files.

[hpoettker]

Sorry for the spam from the failed fuzzing. I'll look into running the fuzzer locally and/or on push before opening PRs.

I've probably opened up the problem by removing the bound on the counter i in pass 2 of sas7bcat_parse_value_labels on both label_count_used and label_count_capacity. With the PR, it's now only bound by label_count_used as that is larger by 1 than label_count_capacity for formats with default value.

But I've added the missing checks to guard against buffer overflow now, and the fuzzing is successful.

[hpoettker]

Please post a Fable review for this PR. I'll then address anything that comes up.

PR Summary

The PR now also resolves #378, and I've rebased on dev.

The PR still addresses the previously described problems when reading SAS catalog files:

• it fixes the issue that XLSR records beyond the first page in 64 bit files are currently not found due to a wrong offset,
• it skips the "informats" in the file, which currently lead to parsing errors
• it skips the default values of custom formats also in the cases where the default value is not written last

Testing

I've added a 64-bit mode to the sas7bcat writer and the test suite for sas7bcat files.

For the long value strings in formats from #378, I've added a dedicated unit test. For the default values to be skipped I created a test resource with Linux 64bit and the following SAS code:

proc format;
  value yesno
    other = 'Unknown'
    1 = 'Yes'
    2 = 'No';
run;

Fuzzer

I had a few run-ins with the fuzzer. One problem was that in the OSS Fuzz build script, there is a sas7bdat* glob but not a corresponding sas7bcat* glob in the paths.

That's why I retained the value sas7bcat for RT_FORMAT_SAS7BCAT_32BIT in file_extension in test_read.c instead of changing it to sas7bcat32, which would be more in line with the values for sas7bdat files.

I can also open a PR in the OSS Fuzz repo if needed.

Outlook

Handling format default values or informats in ReadStat properly would be nice but requires changes or additions to the APIs. I might come back to it later as I've already reverse-engineered the catalog file format for informats.

On the value side, SAS formats can also have ranges instead of just fixed values, which would require further API changes/additions to accomodate.

[evanmiller]

Code Review: PR #370 — Read 64-bit multi-page SAS catalog files and skip format default values

Review of WizardMac/ReadStat#370 (head ddf4d67, base 78bb8a9), ranked most severe first. Findings were checked by building dev and the PR branch in separate worktrees, running make check on the PR (4/4 pass), and linking a small catalog-dump harness and a small writer harness against each build.

Summary

The PR does three things to the sas7bcat reader: it looks for continuation XLSR pages at offset 32 instead of 16 in 64-bit files, it skips informats (names beginning with @), and it reads two new block-header fields — the 1-based index of the other= default label and a string-value offset — so that a default label is skipped no matter where it sits physically. The writer gains a 64-bit mode and the catalog tests now run in both bitnesses.

The default-label fix is real and verified: on the new format_with_default.sas7bcat resource, dev reports YESNO 1 → "Unknown" and YESNO 2 → "Yes" (labels shifted by one), while the PR reports 1 → "Yes", 2 → "No". On the two real 64-bit catalogs I have locally (3 and 302 value labels, produced by SAS) the PR's output is byte-identical to dev's, so there is no regression on those. The code is sound and I would merge it after #1 and #2 below; #3 is a request for evidence rather than a defect I can demonstrate.

Correctness

1. Writer copies the wrong half of int64_t locals on big-endian hosts

src/sas/readstat_sas7bcat_write.c:50 and :176

The PR widens two locals to int64_t and then memcpys sizeof(int32_t) bytes from them in the 32-bit branch:

int64_t count = r_label_set->value_labels_count;
...
memcpy(&block->data[38], &count, sizeof(int32_t));   // 32-bit branch
...
int64_t block_idx = 4;
...
memcpy(&xlsr[4], &block_idx, sizeof(int32_t));       // 32-bit branch

The writer emits native byte order (.endian = machine_is_little_endian() ? ...), and on a big-endian host the low four bytes of an int64_t are at &count + 4, so both copies write the zero high word: label count 0 and XLSR page 0. Before the PR both variables were int32_t and this was correct. Keep a separate int32_t for the 32-bit branch (or copy from (int32_t)count). Not testable on this machine, but it is a straightforward read of the code.

2. Only the first label set survives a round trip (pre-existing, but the PR rewrites this code)

src/sas/readstat_sas7bcat_write.c:189 (XLSR loop) vs :239 (page-3 loop)

The XLSR loop advances block_off += blocks[i]->len, but the page-3 loop lays each block out as chain-link header (16 or 32 bytes) + data, so every XLSR entry after the first points header_size × i bytes short of its block. Verified with a writer harness that adds three label sets (AAA, BBB, $CCC) and reads the file back through the same build:

build	bitness	label sets written	label sets read back
dev	32	3	1 (AAA only)
PR	32	3	1 (AAA only)
PR	64	3	1 (AAA only)

The fix is one line in the XLSR loop, block_off += block_header_size + blocks[i]->len (hoisting block_header_size above the page-1 code). This is pre-existing on dev, but the PR touches exactly these lines to add the 64-bit header size, so it is the natural moment to fix it. Every catalog entry in test_list.h has label_sets_count = 1, which is why the suite never catches it; a two-label-set catalog test would (the harness does compare the total value-label count).

3. The 32-bit header offsets and the string-offset field are verified only against ReadStat's own writer

src/sas/readstat_sas7bcat_read.c:207-214

The reader now trusts four new fields: default_label_pos at 66+pad (32-bit) / 74+pad (64-bit) and string_offset at 104+pad as 2 bytes (32-bit) / 124+pad as 4 bytes (64-bit). The only SAS-produced file in the PR is 64-bit and numeric, and I confirmed by hex-dumping its YESNO block that default_label_pos = 1, string_offset = 0, capacity = 2, used = 3, which is consistent with the reader's model. But:

• No 32-bit SAS-produced catalog is tested. The 32-bit round-trip tests pass by construction because the writer leaves both fields zero. If offset 66 or 104 is wrong for real 32-bit files, the effect is not "slightly wrong output" but READSTAT_ERROR_PARSE on files that read fine today, via the new default_label_pos > label_count_used check and the new label_pos == default_label_pos - 1 check in pass 1. That is the one place this PR could regress users.
• No SAS-produced catalog with a character ($) format is tested in either bitness, so string_offset and the "string runs from 22 + string_offset to the end of the entry" layout are unverified against SAS output. The old reader took the last 16 bytes of the entry, which is a different rule whenever an entry is longer than 38 bytes. The two real files I have contain no $ formats either, so I could not check this myself.
• The 2-byte vs 4-byte read of string_offset between bitnesses is asymmetric with the other fields; worth a comment on where that came from.

Ask: which real files were used to derive the 32-bit offsets and the string layout, and can a small SAS-produced 32-bit catalog and a $-format catalog be added under resources/? Both would be cheap to check in and would make the offsets regression-proof.

4. Multi-page XLSR fix has no coverage

src/sas/readstat_sas7bcat_read.c:523

The change from page[16] to page[32] for 64-bit files is plausible (it matches the 64-bit page header size used by the sas7bdat reader), but nothing exercises it: the new resource has 4 pages with both XLSR records on page 1, the two real files here are the same, and the writer only ever emits XLSR records on page 1. If a real multi-page file is what motivated the fix, a trimmed copy would make a good resource; otherwise a note in the PR that it was verified manually would do.

Robustness

5. Labels no value entry points at are silently paired with the first value entry

src/sas/readstat_sas7bcat_read.c:94

Dropping i < label_count_capacity from the pass-2 loop is required by the fix (the resource has 3 labels but only 2 value entries), and it is memory-safe because value_offset is calloc'd and every offset is bounds-checked. But an unset slot is 0, so any non-default label index that no value entry referenced now reads its value from entry 0 and is emitted with that value. Initialising value_offset to UINT32_MAX and treating an unset non-default index as a parse error (or skipping it) would make corrupt or unexpected files fail loudly instead of emitting a duplicate value. Also, label_count_used and default_label_pos are uint64_t in the caller but int parameters here (pre-existing pattern); harmless today because readstat_calloc refuses absurd counts, but the truncation is silent.

6. int16_t value_entry_len can overflow for long string keys

src/sas/readstat_sas7bcat_write.c:79

Variable-length value entries are new in this PR. readstat_label_string_value places no limit on key length, so a key over about 32 KB wraps value_entry_len and the reader's value_entry_len < string_start check then rejects the block. Either clamp the key at write time or reject it with an error. Related and pre-existing: int16_t block_len and the silent break when a block does not fit the page mean an oversized label set is dropped without an error.

Questions and nits

• Informat skip (readstat_sas7bcat_read.c:60): the $ test runs before the @ test. If SAS stores character informats with a leading $ ($@NAME or similar), they would be parsed as string formats rather than skipped. Do you have an example of how a character informat is named in the catalog?
• int8_t offset = ctx->u64 ? 32 : 16; at :523 — plain int is clearer and avoids the promotion in ctx->page_size - offset.
• off_t for offsets into a 32-byte stack array (:222-224) — size_t or int is more conventional.
• test_read.c:44: "sas7bcat64" as the 64-bit extension is consistent with sas7bdat64, fine.

Verification log

• make check on the PR worktree: test_readstat, test_dta_days, test_sav_date, test_double_decimals all pass.
• format_with_default.sas7bcat: 64-bit, pad1 = 4, little-endian, header 8192, page size 4096, 4 pages, both XLSR records on page 1. YESNO block at file offset 0x5040: capacity = 2, used = 3, default_label_pos = 1, string_offset = 0; labels stored in order Unknown, Yes, No; value entries are 54 bytes each and reference label indices 1 and 2.
• Dump harness, dev vs PR: ~/Downloads/test/formats.sas7bcat (64-bit, 3 labels) identical; ~/Downloads/test2/formats2.sas7bcat (64-bit, 302 labels, 3 formats, no $ formats, no informats) identical; the new resource differs as described in the summary.
• Writer harness with three label sets: see table under Compilation warnings #2.

[hpoettker]

Thanks for picking up parts of this PR already in the latest wave of commits on dev!

The PR is now rebased on #394, which fixes the currently broken build.

I've replaced the previous sample catalog file with a 32- and 64-bit version of a file that has been produced with the following SAS code:

proc format;
  value $yesno
    other = 'Unknown'
    'Yes, I could not agree more' = 'True'
    'No, not really' = 'False';
run;

This way, it tests both the skipping of default values and long value strings.

I have addressed the feedback from the review except points 4 and 6. They are both about larger catalog files. How they should be written by the writer is a topic of its own. I have validated the reader for larger files manually with real-world catalog files. I would refrain from committing larger binary sample files to the repository at this point as this comes with problems down the road.

I can't promise anything but I'll revisit the test harness for catalog files if I get to reading (instead of skipping) the default values, informats, and value ranges.