[soloturn]

AI-assisted change proposal. Filed by agent driven by @soloturn via GDD.

Summary

• Opening a ZIM file from a file descriptor faked a path like /dev/fd/12 and reopened it; reopening that path can fail with a permission error even though the original descriptor still works. Duplicate the descriptor instead.
• Three ways to get an item's data directly:

// before this PR: reopen by path -- fails on some Android fds
auto info = item.getDirectAccessInformation();
int fd = open(info.filename);

// as first shipped here: two calls, two methods
auto info = item.getDirectAccessInformation();  // offset only
int fd = item.getDirectAccessFd();                // fd only

// now: one call, one method
auto info = item.getDirectAccessFd();             // fd + offset together

• getDirectAccessInformation() is now DEPRECATED on POSIX, where the fd-based replacement exists. Windows has no fd-based archive opening, so it keeps the plain declaration there and stays the only option.

Test plan

• Full local build + meson test (POSIX). Pre-existing, unrelated multi-part test-data failures excluded — same on this branch before these changes.

Related

• Review thread with @veloman-yunkan and @kelson42.

[soloturn]

@ojwb since this touches Xapian::Database(int fd) semantics directly (getDbFromAccessInfo() in src/tools.cpp) -- would value your input on whether dup()'ing a descriptor we already hold, instead of reopening a synthesized /dev/fd/ path, is the right way to hand Xapian an fd here, and whether that has any implications on Xapian's side (e.g. around the fd ownership Xapian::Database(int) takes).

[ojwb]

since this touches Xapian::Database(int fd) semantics directly (getDbFromAccessInfo() in src/tools.cpp) -- would value your input on whether dup()'ing a descriptor we already hold, instead of reopening a synthesized /dev/fd/ path, is the right way to hand Xapian an fd here, and whether that has any implications on Xapian's side (e.g. around the fd ownership Xapian::Database(int) takes).

The only potential problem I can see is that dup() creates a file descriptor which shares a file description with the file descriptor passed in, and that means they share a file position, which may matter if you continue to use the original file descriptor and call lseek() on it.

Xapian will use pread() and pwrite() where available, in which case it doesn't use the the file position so everything should be fine. These functions were added in POSIX.1-2001 so I'd expect all Unix-like platforms to support them by now. A possible risk is that they might be badly emulated somewhere by internally calling lseek() followed by read()/write() (this is what glibc did for Linux kernels < 2.1.60).

The file status flags are also shared, but that seems unlikely to cause problems.

/dev/fd/N is also not universally portable though, so using dup() seems the better option here.

[ojwb]

Xapian will use pread() and pwrite() where available, in which case it doesn't use the the file position so everything should be fine.

Hmm, though it looks like reading the version file uses lseek(). That mainly happens when you open the database, and at that point the current file position is relevant because it points to the start of the database (so you can embed within a larger file, like the zim format does IIUC; we chose this over passing in an off_t parameter because the size of off_t can vary on some platforms depending on compilation options, which would cause incompatibilities in the libxapian ABI). You certainly need to leave the file position alone until the call to open the database returns.

However it looks like Database::reopen() will also call lseek(). It's probably not a useful call on an embedded database, but generic database handling code might call it.

Once we know the offset we can use pread() instead so I'll take a look at reducing use of lseek() down to a single call to read the current file position on opening (I've opened https://trac.xapian.org/ticket/854). I'll also clarify the documentation to discuss use of dup().

There are two potential issues:

• Xapian re-reading the version file will change the file position for the caller too.
• If the caller is multithreaded then it could lseek() its fd between Xapian calling lseek() and read() on the dup-ed fd so Xapian could read from the wrong part of the file.

(If the caller - libzim here - only uses pread() itself then neither of these are a problem, assuming pread() is implemented with correct POSIX semantics at least.)

I think for libzim you can likely still use dup() without problems, though I'd suggest checking if you're calling Database::reopen() on the resultant Database object (if so, I think those calls are probably no-ops and can just be removed).

[kelson42]

@veloman-yunkan This is very touchy but worth a review IMHO as it would be very beneficial to get rid of the current jack.

One additional thing I worry about is the handling of ZIM chunks... Really concerned how this piece of code coop with this scenario.

[Copilot]

Pull request overview

This PR addresses failures when reopening fd-backed archives via synthetic /dev/fd/<n> paths by switching internal consumers to duplicate the already-open descriptor with dup(), improving compatibility with Android content-provider file descriptors and similar non-reopenable FDs.

Changes:

• Add a POSIX-only dupFd() helper and use it to open fd-backed FilePart instances without reopening by path.
• Extend ItemDataDirectAccessInfo (POSIX-only) with an optional fd field and plumb it through FileImpl and getDbFromAccessInfo() for Xapian DB opening.
• Add regression tests covering suggestion search on an archive opened from an FD and update direct-access test helpers to prefer dup() when an FD is available.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/fs_unix.h	Declares dupFd(int) helper for POSIX builds.
src/fs_unix.cpp	Implements dupFd() using dup(2) and throws on failure.
src/file_part.h	Switches fd-backed FilePart construction to dupFd(); exposes nativeFd() (POSIX-only).
include/zim/zim.h	Adds POSIX-only fd field and constructor to ItemDataDirectAccessInfo.
src/fileimpl.cpp	Populates ItemDataDirectAccessInfo with nativeFd() when available (POSIX-only).
src/tools.cpp	Updates getDbFromAccessInfo() to prefer dupFd(accessInfo.fd) over reopening by filename (POSIX-only).
test/suggestion.cpp	Adds regression test for suggestion search on FD-opened archives (POSIX-only).
test/archive.cpp	Updates direct-access test helper to dup() the provided fd when present (POSIX-only).

Suppressed comments (1)

test/suggestion.cpp:112

• zim::Archive(int fd) does not take ownership of the passed-in descriptor (see public API docs), so this test should close fd after constructing the archive to avoid leaking descriptors when the loop iterates over multiple test files.

    const int fd = open(testfile.path.c_str(), O_RDONLY);
    ASSERT_NE(-1, fd);
    const zim::Archive archive(fd);
    ASSERT_TRUE(archive.hasTitleIndex());
    const auto mainItem = archive.getMainEntry().getItem(true);

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[soloturn]

AI-assisted change proposal comment. Filed by agent driven by @soloturn via GDD.

Pushed 7daa73b addressing Copilot's 3 inline comments (replied inline on each). Summary of the real one: it correctly flagged that adding fd to ItemDataDirectAccessInfo breaks ABI, since that struct is returned by value from the public Item::getDirectAccessInformation(). Reworked to keep the struct untouched and thread the fd through internal-only signatures instead (FileImpl-private overload + getDbFromAccessInfo()'s own parameter, both non-exported). SuggestionDataBase::initXapianDb() (the actual SuggestionSearcher path issue #852 is about) now reaches the fd via the existing Item::getClusterIndex()/getBlobIndex() ZIM_PRIVATE accessors instead of the public struct.

@ojwb re: Database::reopen() -- checked, libzim never calls it anywhere, so there's nothing to remove there.

@kelson42 re: chunked/multi-part archives -- unaffected by this change either way. getDirectAccessInformation() already declines direct access (falls back to the normal buffered read path) whenever a blob spans more than one file part:

if (++part_its.first != part_its.second) {
 // The content is split on two parts. We cannot have direct access
  return ItemDataDirectAccessInfo();
}

The dup()-vs-reopen choice only ever applies to the single-part case, same as filename-based direct access already did before this PR -- multi-part handling itself isn't touched.

Verified: full local build (meson+ninja, -Wall -Werror, xapian+icu enabled) and meson test, 31/31 passing, including the new FD regression test and the pre-existing getDirectAccessInformationInAnArchiveOpenedByFD.

[codecov]

Codecov Report

❌ Patch coverage is 58.82353% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.92%. Comparing base (e4de4b7) to head (42dd9a1).

Files with missing lines	Patch %	Lines
src/file_part.h	33.33%	0 Missing and 4 partials ⚠️
src/archive.cpp	25.00%	2 Missing and 1 partial ⚠️
src/fileimpl.cpp	57.14%	0 Missing and 3 partials ⚠️
src/fs_unix.cpp	62.50%	0 Missing and 3 partials ⚠️
src/suggestion.cpp	25.00%	0 Missing and 3 partials ⚠️
src/tools.cpp	50.00%	0 Missing and 3 partials ⚠️
src/item.cpp	66.66%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1119      +/-   ##
==========================================
+ Coverage   56.72%   56.92%   +0.20%     
==========================================
  Files         100      100              
  Lines        5056     5094      +38     
  Branches     2239     2252      +13     
==========================================
+ Hits         2868     2900      +32     
+ Misses        714      713       -1     
- Partials     1474     1481       +7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[soloturn]

AI-assisted change proposal comment. Filed by agent driven by @soloturn via GDD.

Follow-up: the previous fix (7daa73b) took fd off the public ItemDataDirectAccessInfo struct to fix the ABI break, but that also broke kiwix/java-libkiwix#151, which needs the fd via the public API for its own JNI use (not just libzim's internal Xapian loading).

Pushed 2d6b49d: a new Item::getDirectAccessFd() method - purely additive (a new non-virtual method changes no object's layout, unlike a struct field), returns an owned, dup()'d fd or -1. This is the "new accessor API" alternative Copilot's original comment suggested. Updated java-libkiwix#151 to use it. Added a regression test (ZimArchive.getDirectAccessFd); full suite still 31/31.

[soloturn]

AI-assisted change proposal comment. Filed by agent driven by @soloturn via GDD.

codecov/patch was failing. Built libzim with coverage instrumentation (-Db_coverage=true), ran the full suite, and found two lines this diff added that no test ever hit:

• dupFd()'s error path (dup() failing).
• getDbFromAccessInfo()'s catch block (dup/open failing).

Added two tests that pass each an invalid fd directly and check they fail gracefully. Both lines are green now.

The other flagged files (fileimpl.cpp, item.cpp, suggestion.cpp) still show "partial" coverage, but every actually new/changed line in this diff is already exercised by the existing tests. What's left is either pre-existing code the diff happened to touch, or the same catch block reached through a deeper call path that would need a fixture with a deliberately corrupted embedded Xapian db to trigger - not worth the fragility for the remaining points.

Pushed 4970765. 32/32 tests still pass.

[soloturn]

AI-assisted change proposal comment. Filed by agent driven by @soloturn via GDD.

Moved the general (unrelated to this PR's own diff) coverage tests out to #1126, back to this PR's original scope. Current state: 4970765 - the ABI-break fix, the new Item::getDirectAccessFd() API, and coverage for the two error paths this PR's own diff introduced (dupFd(), getDbFromAccessInfo()).

[soloturn]

AI-assisted change proposal comment. Filed by agent driven by @soloturn via GDD.

@kelson42 re: ZIM chunk/multi-part handling -- the guard sits in one place, and everything funnels through it.

FileImpl::getDirectAccessInformation(clusterIdx, blobIdx, fd) (src/fileimpl.cpp) does, in order:

1. If the cluster is compressed, bail out with an invalid ItemDataDirectAccessInfo() before fd is touched.
2. If the blob spans more than one file part (multi-part/split ZIM), bail out invalid, again before fd is touched.
3. Only past both guards does it set fd to the part's native fd.

fd starts at -1, so both bail-out paths leave it at -1.

All three places this PR touches read the fd through that same function, so they inherit the guard automatically:

• Item::getDirectAccessFd() checks fd < 0 -> returns -1
• FileImpl::loadXapianDb() checks !accessInfo.isValid() -> returns nullptr
• SuggestionDataBase::initXapianDb() checks the same -> returns

In every bail-out case the caller falls back to the normal buffered read path. That fallback logic is pre-existing and unchanged by this PR -- the PR only adds the fd out-parameter after the existing guards, so it inherits the same safety for free.

[veloman-yunkan]

I started with a dup()-based implementation in #449 but had to resort to the /dev/fd/* hack because of the constraints imposed by the need to account for the presence of the zim::Item::getDirectAccessInformation() method in the public API. We need to get rid of that method (or convert its return type to an fd-based respresentation) - something that we could and should have done in the years that have passed since #449 was merged (three opportunities of major-version increases have been thoughtlessly missed).

This PR can be a starting point for deprecating the getDirectAccessInformation() method, however we must find a viable alternative first.

[kelson42]

@veloman-yunkan I hardly see how this PR could reach completeness and maturity within a few iterations. What should we do with it?

[soloturn]

@veloman-yunkan I hardly see how this PR could reach completeness and maturity within a few iterations. What should we do with it?

@veloman-yunkan comments fixed. especially, getDirectAccessInformation() is not used any more. i successfully tested this pr 1119 end to end by building kiwix-android, and publishing it as release for branch "main-solo". you can e.g. install the apk by installing "obtanium" from f-droid, then point it to https://github.com/soloturn/kiwix-android/releases, set the filter to "main-solo". there are a couple of fixes to kiwix-android necessary to avoid double opening the zim via direct file access, and one can choose the folder via settings (soloturn/kiwix-android@5e310b8).

the kiwix-android build uses the following pr:

• libzim fix: open fd-backed archive parts via dup() instead of reopening by path #1119
• libkiwix fix: serve books added from an fd-backed archive over the local server kiwix/libkiwix#1334
• java-libkiwix feat: expose the fd from ItemDataDirectAccessInfo across JNI kiwix/java-libkiwix#151

the kiwix-android built with these pr benefits from the fd and its dup. it stores the downloaded zim into default folder Documents/Kiwix using androids SAF, asking for permission to store outside the kiwix app folder. that means, uninstall kiwix-android, and reinstall does not remove the zim any more.

[kelson42]

@veloman-yunkan We need to get rid of thsi hack and move forward on fixing bug/limitation on Android. How looks the way forward? Do you develop the fix based on inputbfrom @soloturn? Or do we let him the opportunity to complete this PR in the way you expect it to be?

[soloturn]

@veloman-yunkan We need to get rid of thsi hack and move forward on fixing bug/limitation on Android. How looks the way forward? Do you develop the fix based on inputbfrom @soloturn? Or do we let him the opportunity to complete this PR in the way you expect it to be?

@veloman-yunkan mind having a look, here, and #1126 ?