Skip to content

ModpkgContent::read_wad_overrides silently drops chunks with no recorded path #219

Description

@FrogCsLoL

Ran into this by accident while debugging something unrelated in one of our own packages. The package that triggered it was produced by a bug on our side — Celestial's modpkg export was hex-naming chunks it should have named properly — so the archive is arguably our fault. The consumer behaviour still looks wrong though, and it fails silently, so it seemed worth reporting.

What happens

In ltk_overlay::ModpkgContent::read_wad_overrides, a chunk whose path is absent from chunk_paths() is skipped:

None => {
    tracing::warn!(
        "modpkg chunk {} (layer='{}', wad='{}') has no recorded path; \
         the override will be skipped",
        key.path, layer, wad_name
    );
}

The chunk's path_hash is right there in key.path, and a WAD entry only needs a hash and bytes. The path is required by this function's Vec<(Utf8PathBuf, Vec<u8>)> return type, not by the format.

Is this deliberate?

The warn! means the case was clearly anticipated, so this may well be intended — if so, please close this and I'll adapt on our side. What made us doubt it is that the crate handles the same chunks inconsistently:

  • utils::resolve_chunk_hash explicitly parses a 16-hex-digit stem back into a hash instead of hashing the string, so hex-named chunks are an expected shape here.
  • read_wad_override_file accepts them as its third candidate, commented "this is the case for any asset whose path was never recovered". That function resolves these chunks fine; only read_wad_overrides drops them.

So the same chunk is placeable through one read path and "cannot be placed in a WAD" through the other. If the exclusion is intentional, the second function resolving them looks like the thing that should change instead.

Impact

Any modpkg built from a packed WAD can carry chunks with no recorded path, because a packed WAD stores no paths. Those chunks are dropped with only a warn!, so the package quietly ships less content than it contains.

Worth noting the same archive works correctly through a hash-based consumer that reads pkg.chunks() and uses key.path.value() directly — it never asks for a name, so it never notices one is missing. Only the path-shaped interface loses them.

Suggested fix

Return the hex form of the hash as the relative path instead of skipping:

None => {
    rel_paths.insert(key.path, format!("{:016x}", key.path.value()));
}

resolve_chunk_hash parses a 16-hex stem straight back into the hash rather than hashing the string, so the chunk lands at exactly the right place. No interface change, and it makes pass 1 consistent with pass 2.

A single summary line (N chunk(s) have no recorded path; serving them by hash) reads better than one warning per chunk, and still records that the archive cannot name those assets — which does matter for anything downstream that needs real paths, like repair or repathing tooling.

Versions

Present in ltk_overlay 0.9.7.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions