Skip to content

fix(services/ipmfs): stop stripping the root twice from listed entries - #8070

Merged
erickguan merged 3 commits into
apache:mainfrom
PDGGK:fix-ipmfs-entry-path
Aug 15, 2026
Merged

fix(services/ipmfs): stop stripping the root twice from listed entries#8070
erickguan merged 3 commits into
apache:mainfrom
PDGGK:fix-ipmfs-entry-path

Conversation

@PDGGK

@PDGGK PDGGK commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

None — found while reading the service.

Rationale for this change

The listing loop concatenates the path being listed with a files/ls name, then runs the result through build_rel_path a second time:

for object in entries_body.entries.unwrap_or_default() {
    let path = match object.mode() {
        EntryMode::FILE => format!("{}{}", self.path, object.name),
        EntryMode::DIR => format!("{}{}/", self.path, object.name),
        EntryMode::Unknown => unreachable!(),
    };

    let path = build_rel_path(&self.root, &path);   // <- the root is not in `path`

self.path is the path the operator handed the service, so it is already relative to the root — IpmfsCore::ipmfs_ls is what turns it into a rooted absolute path for the request:

pub(crate) async fn ipmfs_ls(&self, ctx: &OperationContext, path: &str) -> Result<Response<Buffer>> {
    let p = build_rooted_abs_path(&self.root, path);

So the concatenation is root-relative before that call, and the call strips a prefix that is not there.

Under the default root / the second strip cancels out, which is why this has gone unnoticed. Under any other root, measured in release (cargo test --release):

root listing name result
/ dir/ a dir/a
/abc/ dir/ a a — the directory is dropped from every entry path
/abc/ the root itself, so self.path is / a panic: start byte index 5 is out of bounds for string of length 2

In a debug build both of the last two trip the debug_assert! inside build_rel_path instead.

What changes are included in this PR?

The construction moves into build_entry_path, which takes no root at all — an entry path does not depend on one. The only thing it has to handle is that self.path is "/" when the root itself is listed, and an entry path carries no leading slash.

The self-entry twenty lines above is deliberately left alone: it does build_abs_path and then build_rel_path, a genuine round trip, and is correct.

Tests

Three unit tests. Restoring the old expression shows the defect and why it survived:

old expression, service root result
/ 4 passed — the double strip cancels out
/abc/ 3 failed, 1 passed

cargo test -p opendal-service-ipmfs --lib → 4 passed. cargo fmt --all -- --check and cargo clippy -p opendal-service-ipmfs --all-targets both clean.

Note for rebasing

#7801 renames this very call to build_relative_path. If that lands first this needs a one-word rebase — though the call disappears here, which is the point.

Are there any user-facing changes?

Yes, for services-ipmfs with a root other than /: listed entry paths keep the directory they are in, and listing the root no longer panics. Behaviour under the default root / is unchanged.

The listing loop concatenates the path being listed with a files/ls
name, then runs the result through build_rel_path a second time:

    let path = match object.mode() {
        EntryMode::FILE => format!("{}{}", self.path, object.name),
        ...
    };
    let path = build_rel_path(&self.root, &path);

self.path is what the operator handed the service, so it is already
relative to the root -- IpmfsCore::ipmfs_ls is what turns it into a
rooted absolute path for the request. The concatenation is therefore
root-relative before that call, and the call removes a prefix that is
not there.

Under the default root "/" the second strip happens to cancel out, which
is why this has gone unnoticed. Under any other root, in release:

    root "/abc/", listing "dir/", name "a"
        build_rel_path("/abc/", "dir/a") -> "a"
        the directory is dropped from every entry path

    root "/abc/", listing the root itself (self.path is "/"), name "a"
        build_rel_path("/abc/", "/a")
        panicked: start byte index 5 is out of bounds for string of
        length 2

In a debug build both cases trip the debug_assert! inside build_rel_path
instead.

The construction moves into build_entry_path, which takes no root at
all -- an entry path does not depend on one. The only thing it has to
handle is that self.path is "/" when the root itself is listed, and an
entry path carries no leading slash.

The self-entry twenty lines above is left alone: it does
build_abs_path then build_rel_path, a deliberate round trip, and is
correct.

Three unit tests. Putting the old expression back passes all of them
with a root of "/" and fails three of them with a root of "/abc/" --
which is the shape of the bug.

Note for rebasing: apache#7801 renames this very call to
build_relative_path. If that lands first this needs a one-word rebase;
the call itself goes away here.
@PDGGK
PDGGK requested a review from Xuanwo as a code owner August 14, 2026 14:28
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. releases-note/fix The PR fixes a bug or has a title that begins with "fix" labels Aug 14, 2026
Matching the shape asked for on apache#8068, apache#8069 and apache#8071 rather than
waiting to be asked again: the extracted helper goes, the construction
is inline, and the test module goes with it.

The fix is unchanged -- the concatenation is already relative to the
root, so the second build_rel_path is what truncated it, and "/" as the
listed path must not become a leading slash on the entry.
@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Aug 15, 2026
@PDGGK

PDGGK commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Slimmed this to match the shape you asked for on #8068, #8069 and #8071, rather than making you say it again.

The extracted helper is gone and the construction is inline; the test module went with it. The diff is now +10/-4.

The fix itself is unchanged: self.path is already relative to the root — IpmfsCore::ipmfs_ls is what roots it for the request — so the second build_rel_path was stripping a prefix that is not there. Under the default root / that strip cancels out, which is why it went unnoticed; under any other root it truncated every entry path, and listing the root panicked outright with start byte index 5 is out of bounds for string of length 2.

The removed tests were the thing pinning that the entry path does not depend on the service root. Your call, not asking to keep them.

Comment thread core/services/ipmfs/src/lister.rs Outdated
ctx.done = true;

for object in entries_body.entries.unwrap_or_default() {
// `self.path` is "/" when the root itself is listed, and an entry path carries

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.

Remove this comment.

@PDGGK

PDGGK commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Removed. The diff is now +7/-4 — the prefix binding and the dropped build_rel_path line.

The reasoning stays in the commit message and the PR description, so it is still findable without sitting in the loop body.

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Aug 15, 2026
@erickguan
erickguan merged commit 7144454 into apache:main Aug 15, 2026
108 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer releases-note/fix The PR fixes a bug or has a title that begins with "fix" size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants