Several fstapi correctness and memory-safety bugs

[Rodrigodd]

Hi. While I was working on fixing some bugs in a Rust implementation of an FST writer with the help of a fuzzer (see ekiwi/fst-writer#7), I found one or two bugs in libfst itself. When I started working on fixing those bugs, with the help of AI, adding coverage tests, etc., I ended up finding a lot more, which I listed below.

I am still working on it, but I am planning to start splitting the fixes into multiple PRs and submit them slowly as I review and clean them up. Before submitting 10+ PRs, though, I thought it would be good to ask the maintainers whether you are fine with that, or would prefer I do something different.

My work in progress is currently in this branch: https://github.com/Rodrigodd/libfst/tree/fix-writer-and-reader-bugs

Below is a list of the bugs I found and fixed so far. Each commit in the branch has a longer (although AI-generated) description of the bug and of the fix applied, and almost all of them come with a regression test that reproduces the bug.

---

Writer bugs:

• Flush with nothing pending appends to the current time chain, shifting every later timestamp in the section.
• A section that never receives a value change leaves a stray zero-length header, making the rest of the file unreachable.
• A value change emitted before the first time change is written only to the frame and never reported by fstReaderIterBlocks2().
• A variable-length value change emitted before the first time change is silently dropped.
• Variable-length value changes keep growing the pending buffer after the dump size limit is reached.
• fstWriterEmitValueChangeVec64() truncates each word to 32 bits, corrupting any value wider than 64 bits.
• fstWriterEmitValueChangeVec32/64() read one word past the end when the width is a whole number of words.
• Handle zero passes the bounds check in the value-change emitters and underflows into an out-of-bounds index.

Reader bugs:

• fstReaderProcessHier() reallocates the facility process mask from scratch, discarding every mask bit the caller set.
• A file whose hierarchy cannot be read returns a context with a NULL process mask that crashes on first use.
• A file with no value change section never rewinds its hierarchy stream, so it reports an empty hierarchy.
• fstReaderSetFacProcessMaskAll()/fstReaderClrFacProcessMaskAll() memset through a NULL mask.
• Reals reconstructed from a section frame are missing the r prefix that the value-change path emits.
• Compressed value change chains are decompressed with a length that includes the chain's own length varint (heap overflow with FASTLZ).
• The section peek in fstReaderGetValueFromHandleAtTime() seeks one byte into the current section instead of the next one, and doesn't restore state.

Supporting work (not fixes, but part of the branch):

• fastlz kept on its portable path, to avoid unaligned-access UB reports.
• Regression tests for each fix, plus API coverage tests (fstapi.c line coverage 43.9% → 68.8%).
• A clang source-based coverage build (-Dllvm_coverage=true, coverage-json target).
• An ASan/UBSan CI job.
• A round-trip libFuzzer target that models the writer and checks every reader entry point against it.

[tbybell]

I'll examine whatever you put out there. I'm more interested in reader bugs than writer ones as an initial drop. The reason I say that is because there is a quite limited set of software out there interfacing to the writer, so updated reader code probably would have more immediate impact on users.

For potential security cases, openly publishing information that could be used as exploit conditions before privately contacting developers first is in very bad form.

[Rodrigodd]

For potential security cases, openly publishing information that could be used as exploit conditions before privately contacting developers first is in very bad form.

Sorry about that. I didn't spot a security policy in the project, and from the lack of tests and the low hanging bugs I just assumed security was not much of a concern here. That was careless of me to assume, more so because I am not familiar with the GTKWave community. From a quick search I could not find any point of contact for security reports, could you give me a link for where I can find one?

I'm more interested in reader bugs than writer ones as an initial drop.

Ok. So far I have only fed the reader files produced by the writer, so I will also invest work in testing input validation against malformed files.

[tbybell]

No problem...I don't go crazy about security, but wrote what I did because on other projects, their developers might really take offense. Note that if you look through old issues, you'll see there are TALOS fixes in the library. Ralf (below) and I are the two main points of contact for this project.

Note that I ran code analysis on fstapi.c and it discovered all the TALOS-2023-1789 fixes were missing from fstReaderGetValueFromHandleAtTime(), so I checked in the fixes for those.

@rfuest, it's been a while since I used github here so I completely forgot I was supposed to do a pull request such that the automated tools could do their build checking, but the unconditional push I did should be OK across all platforms as the code is analogous cut and pastes from one function into the rvat one.

[Rodrigodd]

Hi @tbybell, last Thursday I sent you an email with some more issues I found that may be of concern to you. Just checking if you received it. I had @rfuest’s email in copy too. I sent it to the email addresses I found in the git log.

[tbybell]

Recovered the email as the spam filter ate it on me. Thanks for that heads up it was there!