Skip to content

Initial support for cfg.choice for sub-files ##bin#25860

Open
trufae wants to merge 1 commit intomasterfrom
binchoice
Open

Initial support for cfg.choice for sub-files ##bin#25860
trufae wants to merge 1 commit intomasterfrom
binchoice

Conversation

@trufae
Copy link
Copy Markdown
Collaborator

@trufae trufae commented Apr 22, 2026

  • Mark this if you consider it ready to merge
  • I've added tests (optional)
  • I wrote some lines in the book (optional)

Description

Comment thread libr/include/r_io.h
bool (*check)(RIO *io, const char *, bool many);
// Optional: enumerate sub-entries of a container URI without opening them all.
// Returns RList<RIOSubEntry*>, or NULL if the URI has none / plugin doesn't implement it.
RList* (*list_subs)(RIO *io, const char *uri);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should exist, just like the zip plugin shouldn't exist. This feels like it should be part of r_fs and that r_fs should operate on the underlying RIODesc and expose subfiles as new RIODesc with a RIOPlugin that lives inside of r_fs. However this probably requires a huge refactoring and I get why this cannot be done easily.

@trufae
Copy link
Copy Markdown
Collaborator Author

trufae commented Apr 22, 2026

PR #25860 — Initial support for cfg.choice for sub-files
Verdict: ⚠️ Needs Changes

The design is sound — two-tier sub-binary discovery (IO-level list_subs + bin-level xtr extractors) is a clean layering. The new oS command and the cfg.choice interactive prompt are useful user-facing features. ABI bump is correctly done. But there are a few issues to address:

🔴 High: Dangling io->desc after probe open
In r_core_file_subs(), r_io_open_nomap() is used to probe the file for xtr slices, then the descriptor is closed. But r_io_open_nomap() has a side-effect — it sets io->desc = desc when autofd is true or io->desc is NULL. After r_io_desc_close() frees the descriptor, core->io->desc becomes a dangling pointer. The next operation touching io->desc (e.g. the real r_io_open_nomap() in r_core_file_open()) will hit a use-after-free.

Fix: Save/restore io->desc around the probe, or NULL it out after close if it was the probe desc.

🔴 High: oS text output duplicates print_subs_table() with different format
The oS command in cmd_open.inc.c has its own text-mode loop that's missing the "kind" column and uses a different column order than print_subs_table() in file_subs.c. Should just call print_subs_table() to avoid divergence.

🟠 Medium: xtr_extractall int truncation
The (int)sz cast on the r_buf_read_at return comparison silently fails for buffers > 2GB. Use (st64)sz or add an explicit size guard.

🟠 Medium: Fake RBinFile swap is fragile
The R_NEW0(RBinFile) + bin->cur swap trick works today because current xtr plugins only touch xtr_obj, but it's brittle — any plugin accessing bo, file, or buf fields will crash. Worth a // HACK: comment at minimum, and ideally the xtr API should not require bin->cur.

🟡 Low: apply_envs naming is misleading
The function sets radare2 config keys via r_config_set(), not OS environment variables. The envs field name and apply_envs function name are confusing — consider configs/apply_configs.

Design observations (non-blocking)
The probe-then-open pattern means the file is opened twice (once for discovery, once for real). Acceptable for v1 but worth noting for optimization later.

For zip-inside-fatmach0 scenarios, the two discovery layers don't compose (IO-level sees zip entries, xtr sees mach-o slices, but you can't xtr-probe an individual zip entry). This is fine for the intended use cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants