capstone, upx, fq: complete the reverse-engineering workbench - #549
Merged
Conversation
The disassembler engine underneath most open reverse-engineering tooling, packaged as a shared library so consumers can link it rather than vendor it. BUILD_SHARED_LIBS defaults to OFF upstream (and BUILD_STATIC_LIBS to ON), so a stock build ships libcapstone.a only. Anything linking that absorbs capstone statically, which puts a capstone CVE beyond pkgscan's reach entirely — nothing in the consumer's tree would name capstone at all. Shared on, static off, and the `ships_shared_lib` test pins it rather than trusting the flag. Tests disassemble real bytes across five architectures, including two fringe ones (riscv64, ppc64be), because CAPSTONE_ARCHITECTURE_DEFAULT gates every per-arch support option at once: turning it off yields a cstool that builds, runs, and reports the same version while silently having lost whole architectures. A deliberate no-such-arch case is included as a control, so the greps cannot pass on an error message. 5.0.9 rather than 6.0.0-Alpha10: the 6.0.0 alphas are tagged prerelease=false on GitHub but are alphas by name. Note rizin does NOT consume this — it pins capstone "next" (the unreleased v6) and vendors it, as recorded in that package.
The standard tool for unpacking UPX-compressed binaries, which is most of what an RE workbench wants it for. Three things worth a reviewer's attention: Vendored compressors are STRUCTURAL here, not a config choice. vendor/ carries ucl, lzma-sdk, zlib, zstd and bzip2, and the CMake build offers no find_package() or system-lib option at all — unlike rizin, where seventeen use_sys_* flags existed and flipping them was the whole job. It is not an oversight either: upx's output is read back by a decompression stub welded onto the packed executable, so compressor and stub must agree bit-for-bit. The consequence is stated rather than left to be discovered — those five libraries are invisible to pkgscan on this package, permanently. The source is not fetched with extract = true. Upstream compresses the -src tarball with an xz SHA-256 integrity check (xz --list reports Check: SHA-256, not the usual CRC64) and the fetcher's decoder rejects it: compression error: Unsupported SHA-256 checksum (not yet implemented) build.sh unpacks it with the system tar instead. runtime_deps carries libstdc++/libgcc beyond glibc — found by the checker, not guessed: with glibc alone even `upx --version-short` died with "error while loading shared libraries: libstdc++.so.6". The test packs a real binary, EXECUTES the packed copy and compares its output to the original, then unpacks and cmp's byte-for-byte. Nothing cheaper works: `upx --version` runs without touching a compressor, and `upx --best` will happily produce a file that is smaller and completely unrunnable if the per-architecture stub is wrong.
jq for binaries: ~120 format decoders behind a jq expression language, which makes "what is in this file, structurally" scriptable across a corpus rather than a thing you do one file at a time in a hex editor. Built CGO_ENABLED=0 (upstream's own setting), so the result is a pure-Go static binary with no DT_NEEDED — hence the empty runtime_deps, same shape as helm. If CGO ever creeps back in, the missing-runtime_deps checker is what notices. No -X version stamping: fq keeps its version as a plain const in fq.go, and the Go linker only rewrites vars — a -ldflags -X aimed at a const is silently ignored, which is the kind of thing that looks maintained and does nothing. The test decodes rather than smoke-tests. `fq --version` and even `fq -n '1+1'` pass on a build whose format decoders are all broken, since the jq engine and the decoders are separate packages and only the decoders are the reason to ship this. So it asserts on a gzip's INFLATED payload — proving the DEFLATE decoder ran and returned the original bytes — not merely on the header field, and adds a second decoder (ELF) over a real file. The expected ELF machine is derived from uname rather than hardcoded, after an earlier package in this loadout pinned an architecture literal and only failed once it reached amd64 CI.
Contributor
📝 WalkthroughWalkthroughThis PR adds package specifications and build scripts for Capstone, fq, and UPX. The packages define reproducible builds, outputs, metadata, runtime settings, and validation tests. ChangesPackage additions
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/capstone/build.ncl`:
- Around line 11-18: Remove the glibc entry from the build_deps list in the
build configuration, leaving its existing runtime_deps declaration unchanged and
avoiding duplication.
- Around line 39-43: Update the license_spdx declaration in the build
configuration from the single BSD-3-Clause identifier to the combined SPDX
expression BSD-3-Clause AND NCSA, reflecting both the project license and
LLVM-derived decoder tables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 123ecdd1-10bc-475a-a10d-57c30fcf2017
📒 Files selected for processing (6)
packages/capstone/build.nclpackages/capstone/build.shpackages/fq/build.nclpackages/fq/build.shpackages/upx/build.nclpackages/upx/build.sh
Comment on lines
+11
to
+18
| build_deps = [ | ||
| { file = "build.sh" } | Local, | ||
| base, | ||
| cmake, | ||
| ninja, | ||
| pkgconf, | ||
| toolchain, | ||
| glibc, |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove glibc from build_deps.
Line 34 already declares glibc in runtime_deps. Do not duplicate runtime dependencies in build_deps.
As per coding guidelines, runtime dependencies do not need to be duplicated in build_deps.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/capstone/build.ncl` around lines 11 - 18, Remove the glibc entry
from the build_deps list in the build configuration, leaving its existing
runtime_deps declaration unchanged and avoiding duplication.
Source: Coding guidelines
twitchyliquid64
approved these changes
Jul 31, 2026
Both packages declared a single licence where the shipped artifact carries
more than one. The repo already models this — 54 packages use compound SPDX
expressions — so these were simply wrong, not stylistically different.
capstone: the tree ships LICENSE.TXT (BSD-3-Clause) AND LICENSE_LLVM.TXT for
the decoders generated from LLVM's tables. My comment claimed that second file
was "Apache-2.0 WITH LLVM-exception". It is not: capstone 5.x predates LLVM's
relicensing and the file is the old LLVM Release License, i.e. University of
Illinois/NCSA. Now (BSD-3-Clause AND NCSA), with the comment corrected.
upx: was GPL-2.0-or-later alone, which omits the vendored code compiled into
the binary. Established from the build rather than from the vendor/ listing,
which turns out to overstate it in one direction and understate it in another:
ucl GPL-2.0-or-later linked (own CMake target)
zlib Zlib linked (own CMake target)
lzma-sdk public domain linked INVISIBLY — compress_lzma.cpp #includes
its .cpp files directly, so it never appears as
its own objects in the ninja log
doctest MIT linked the same way, via src/check/dt_impl.cpp;
this release binary carries its test framework
bzip2 — NOT linked; CMakeLists.txt:269 hard-disables it
zstd — NOT linked; CMakeLists.txt:270 hard-disables it
So (GPL-2.0-or-later AND Zlib AND MIT); the LZMA SDK is public domain and adds
no conditions and no SPDX identifier. The earlier claim that five vendored
libraries ride invisibly in this package was wrong in both directions — it is
three, and two of the five named are not in the binary at all.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/capstone/build.ncl (1)
107-109: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRestrict the
pkg-configlookup to the package output.
PKG_CONFIG_PATHadds a directory but does not suppress default directories. Require/usr/lib/pkgconfig/capstone.pcand setPKG_CONFIG_LIBDIRto prevent a false positive.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/capstone/build.ncl` around lines 107 - 109, Update the pkg-config command in the capstone package check to require the exact /usr/lib/pkgconfig/capstone.pc file and set PKG_CONFIG_LIBDIR to /usr/lib/pkgconfig, preventing lookup in default directories while preserving the existing failure behavior.packages/upx/build.ncl (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicate
glibcbuild dependency.
glibcis already declared inruntime_depsat Line [46]. Remove the Line [17] entry and keep the runtime declaration.As per coding guidelines, runtime dependencies do not need to be duplicated in
build_deps.Proposed cleanup
toolchain, - glibc, {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/upx/build.ncl` at line 17, Remove the duplicate glibc entry from the build_deps declaration in build.ncl, while preserving the existing glibc entry in runtime_deps.Source: Coding guidelines
packages/upx/build.sh (1)
43-50: 🔒 Security & Privacy | 🔵 TrivialTrack vulnerabilities in bundled components outside
pkgscan.The block at Line [43] through Line [50] states that linked vendor code is invisible to
pkgscan. Add a maintained component inventory or CI advisory check for the bundled revisions, especially zlib. Otherwise, a security fix can be missed when UPX is rebuilt.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/upx/build.sh` around lines 43 - 50, Extend the build/security tracking represented by the comments in packages/upx/build.sh with a maintained inventory or CI advisory check for the bundled linked libraries, explicitly including zlib and their revisions. Ensure vulnerability checks run independently of pkgscan so fixes in bundled components are detected when UPX is rebuilt.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/capstone/build.ncl`:
- Around line 107-109: Update the pkg-config command in the capstone package
check to require the exact /usr/lib/pkgconfig/capstone.pc file and set
PKG_CONFIG_LIBDIR to /usr/lib/pkgconfig, preventing lookup in default
directories while preserving the existing failure behavior.
In `@packages/upx/build.ncl`:
- Line 17: Remove the duplicate glibc entry from the build_deps declaration in
build.ncl, while preserving the existing glibc entry in runtime_deps.
In `@packages/upx/build.sh`:
- Around line 43-50: Extend the build/security tracking represented by the
comments in packages/upx/build.sh with a maintained inventory or CI advisory
check for the bundled linked libraries, explicitly including zlib and their
revisions. Ensure vulnerability checks run independently of pkgscan so fixes in
bundled components are detected when UPX is rebuilt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 68bffac8-3bee-46da-b891-bcf669ca47cf
📒 Files selected for processing (3)
packages/capstone/build.nclpackages/upx/build.nclpackages/upx/build.sh
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.
The last three of the reverse-engineering workbench: the disassembler engine, the unpacker, and the binary-format query tool. Independent of #545 (ghidra) and #547 (gef + rizin) — nothing here references them, so this can land in any order.
Each package's interesting decision is in its commit message; the short version:
capstone 5.0.9 — the default is static, and static is invisible
BUILD_SHARED_LIBSdefaults to OFF upstream (BUILD_STATIC_LIBSto ON), so a stock build shipslibcapstone.aonly. Anything linking that absorbs capstone statically, which puts a capstone CVE beyond pkgscan's reach entirely — nothing in the consumer's tree would name capstone at all. Shared on, static off, and a test pins the outcome rather than trusting the flag.5.0.9 rather than 6.0.0-Alpha10: the 6.0.0 alphas are tagged
prerelease=falseon GitHub but are alphas by name. rizin (#547) does not consume this — it pins capstonenext, the unreleased v6, and vendors it; that's recorded in the rizin package and is unchanged by this PR.Licence is
(BSD-3-Clause AND NCSA): the tree shipsLICENSE.TXT(BSD-3-Clause) andLICENSE_LLVM.TXTfor the decoders generated from LLVM's tables. That second file is the old LLVM Release License — University of Illinois/NCSA — because capstone 5.x predates LLVM's relicensing. An earlier revision of this PR called it "Apache-2.0 WITH LLVM-exception" and declared BSD-3-Clause alone; both were wrong.upx 5.2.0 — vendored compressors that cannot be unbundled
The CMake build offers no
find_package()or system-lib option at all — unlike rizin, where seventeenuse_sys_*flags existed and flipping them was the whole job. It isn't an oversight: upx's output is read back by a decompression stub welded onto the packed executable, so compressor and stub must agree bit-for-bit; a system zlib would produce files this upx's own stubs could not unpack.What is actually linked in is narrower than
vendor/suggests, and reading the directory listing gets it wrong in both directions:compress_lzma.cpp#includes its.cppfiles directly, so it never appears as its own objects in the ninja logsrc/check/dt_impl.cpp— this release binary carries its test frameworkCMakeLists.txt:269hard-disables itCMakeLists.txt:270, "currently not used; maybe in UPX version 6"So
license_spdxis(GPL-2.0-or-later AND Zlib AND MIT)— the LZMA SDK is public domain, adding no conditions and no SPDX identifier.The consequence is stated rather than left to be discovered: the three libraries that are linked are invisible to pkgscan on this package, permanently. A CVE in the bundled zlib will not appear against it. Not a TODO anyone can close — flagging it since it's the exact blind spot this distro exists to avoid.
Two more things a reviewer would otherwise trip on:
extract = true. Upstream compresses the-srctarball with an xz SHA-256 integrity check (xz --list→Check: SHA-256, not the usual CRC64) and the fetcher's decoder rejects it outright:compression error: Unsupported SHA-256 checksum (not yet implemented). build.sh unpacks with the system tar.runtime_depsneeds libstdc++/libgcc. Found by the checker, not guessed — with glibc alone, evenupx --version-shortdied witherror while loading shared libraries: libstdc++.so.6.fq 0.17.0 — jq for binaries
~120 format decoders behind a jq expression language.
CGO_ENABLED=0(upstream's own setting) makes it a pure-Go static binary with noDT_NEEDED, hence emptyruntime_deps— same shape ashelm.No
-Xversion stamping: fq keeps its version as a plain const infq.go, and the Go linker only rewrites vars, so a-ldflags -Xaimed at it would be silently ignored. That's the kind of line that looks maintained and does nothing, so it's absent and commented.Tests target each package's actual silent failure
CAPSTONE_ARCHITECTURE_DEFAULTgates every per-arch option at once — cstool still builds, runs, and reports the same version with whole architectures goneupx --bestwill produce a file that is smaller and completely unrunnable if the per-arch stub is wrongcmpbyte-for-bytefq -n '1+1'passes with every decoder brokenAll mutation-tested. Broke one load-bearing assertion in each package simultaneously and confirmed
min checkfailed all three, each naming the exact assertion — then restored and re-verified green. Worth doing:StandaloneTestCheckmaps a cache miss on anytest_deptoSkip, andcheckonly errors onFail, so a green suite does not by itself prove the tests ran.Two bugs in my own tests, both caught by running them:
cstool ppc64isCS_MODE_LITTLE_ENDIANin capstone 5 (ppc64beis the big-endian one) and my vector was the BE encoding; and the fq ELF assertion derives the expected machine fromunamerather than hardcoding it, after an earlier package in this loadout pinned an arch literal and only failed once it reached amd64 CI.Verified
Built and checked entirely through the min-native path — no
minimal packageanywhere:Reproducibility was measured from inside the build sandbox: a
min package buildwrites to the daemon's store inside the VM and populates neither host cache, so the old "hash the tree under~/.cache/minimal/built" method no longer applies. Hashes above are of the installed artifacts, before and after a forced rebuild.Arch note: built and verified on arm64 only — the amd64 path is exercised by CI, and unlike the ghidra PR nothing here compiles per-architecture natives, so the risk is ordinary rather than structural.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Packages
fq0.17.0, a standalone data query and processing tool.Quality Assurance