A group WMO's MOGP runs to EOF, whatever size it declares

MOGP is a group file's last top-level chunk, so EOF is its real end. The walk
trusted the declared size instead, and `parse_group` answers an empty group for
a payload shorter than the 0x44 header — so a group that declares less than it
holds left MOVT/MOVI/MOPY sitting as top-level siblings *after* the payload and
parsed to no vertices, no indices and no batches. A building that draws nothing,
with no error anywhere to say why.

Seven files in this realm's chain do it, and all seven are the winning copy
(`patch-3.mpq`):

  * the six `World\wmo\Outland\Shadowmoon\Slagpit\Shadowmoon_Slagpit01_00{0..5}
    .wmo` declare 0x40, four bytes short of the header, over 33 KiB to 546 KiB
    of geometry;
  * `HumanHouse\WestfallHouse_MediumStraw_000.wmo` declares 0, over 41 KiB.

Measured before and after over the whole corpus: 7 of 7,795 group files parsed
to an empty group, now 0.

`wmo_chunk_overrun.rs` already covered the other direction — Undercity_144
declares one byte *past* EOF — but it only asks for the MOGP header, which
`find_wmo_chunk` clamps and which survives a short declaration too (0x40 still
carries flags, the portal refs and groupLiquid). The geometry is what was lost,
so the new corpus test asks for that: no group file of 1 KiB or more may parse
to zero vertices.

Ported from benilla-bourbon (tripleslash/benilla-bourbon, "a WMO group's MOGP
runs to EOF", credited there to kalistoking), whose `mogp_payload` and unit test
are taken unchanged; the Westfall house and the corpus test are this chain's.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn

crates/benilla-formats/tests/wmo_short_mogp.rs

@@ -0,0 +1,99 @@
+//! A group WMO's MOGP runs to EOF, whatever size it declares.
+//!
+//! MOGP is a group file's last top-level chunk. Seven files in this realm's chain declare less
+//! than they hold, which left their geometry sub-chunks as top-level siblings *after* the declared
+//! payload: the parse answered a group with no vertices, no indices and no batches — a building
+//! that draws nothing, with no error anywhere to say why.
+//!
+//! `wmo_chunk_overrun.rs` covers the other direction (Undercity_144 declares one byte *past* EOF)
+//! and only asks for the MOGP header, which survives either way. This asks for the geometry.
+
+use benilla_formats::Chain;
+use benilla_wmo::{parse_wmo, ParsedWmo};
+
+/// The seven: six Shadowmoon Slagpit groups declaring 0x40 against a 0x44 header, and a Westfall
+/// house group declaring 0. All seven are the winning copy in this chain (`patch-3.mpq`).
+#[test]
+fn the_short_mogp_groups_in_the_chain_yield_their_geometry() {
+    let data = benilla_formats::wow_data_or_skip!();
+    let chain = Chain::open(&data).expect("open vanilla patch chain");
+
+    let mut checked = 0u32;
+    for name in [
+        "World\\wmo\\Outland\\Shadowmoon\\Slagpit\\Shadowmoon_Slagpit01_000.wmo",
+        "World\\wmo\\Outland\\Shadowmoon\\Slagpit\\Shadowmoon_Slagpit01_001.wmo",
+        "World\\wmo\\Outland\\Shadowmoon\\Slagpit\\Shadowmoon_Slagpit01_002.wmo",
+        "World\\wmo\\Outland\\Shadowmoon\\Slagpit\\Shadowmoon_Slagpit01_003.wmo",
+        "World\\wmo\\Outland\\Shadowmoon\\Slagpit\\Shadowmoon_Slagpit01_004.wmo",
+        "World\\wmo\\Outland\\Shadowmoon\\Slagpit\\Shadowmoon_Slagpit01_005.wmo",
+        "HumanHouse\\WestfallHouse_MediumStraw_000.wmo",
+    ] {
+        let Ok(bytes) = chain.read(name) else {
+            continue; // a realm that stops shipping one of these is not a failure here
+        };
+        // The premise: the declared MOGP size really is short of the payload.
+        let declared = u32::from_le_bytes([bytes[0x10], bytes[0x11], bytes[0x12], bytes[0x13]]);
+        assert!(
+            &bytes[0x0c..0x10] == b"PGOM" && 0x14 + (declared as usize) < bytes.len(),
+            "{name}: expected a MOGP declaring less than it holds"
+        );
+        let Ok(ParsedWmo::Group(g)) = parse_wmo(&mut std::io::Cursor::new(bytes.as_slice())) else {
+            panic!("{name}: expected a group");
+        };
+        assert!(
+            !g.vertex_positions.is_empty() && !g.vertex_indices.is_empty(),
+            "{name}: parsed to an empty group ({} verts, {} indices)",
+            g.vertex_positions.len(),
+            g.vertex_indices.len()
+        );
+        checked += 1;
+    }
+    assert!(
+        checked >= 6,
+        "only {checked} of the seven were in the chain"
+    );
+}
+
+/// The invariant the seven broke, over the whole corpus: a group file with real bytes in it parses
+/// to real geometry. 0-byte and stub files ship in the corpus and are not groups with lost
+/// geometry, so the floor is 1 KiB — every one of the seven is 32 KiB or more.
+#[test]
+fn no_substantial_group_wmo_parses_to_an_empty_group() {
+    let data = benilla_formats::wow_data_or_skip!();
+    let mut chain = Chain::open(&data).expect("open vanilla patch chain");
+    let mut paths: Vec<String> = chain
+        .list()
+        .expect("list the patch chain")
+        .into_iter()
+        .map(|e| e.name)
+        .filter(|n| n.to_ascii_lowercase().ends_with(".wmo"))
+        .collect();
+    paths.sort();
+    paths.dedup();
+    assert!(
+        paths.len() > 5000,
+        "expected the full WMO corpus, found {} files",
+        paths.len()
+    );
+
+    let mut checked = 0u32;
+    let mut empty: Vec<String> = Vec::new();
+    for p in &paths {
+        let Ok(b) = chain.read_file(p) else { continue };
+        if b.len() < 1024 || b.len() < 16 || &b[12..16] != b"PGOM" {
+            continue;
+        }
+        checked += 1;
+        match parse_wmo(&mut std::io::Cursor::new(b.as_slice())) {
+            Ok(ParsedWmo::Group(g)) if g.vertex_positions.is_empty() => empty.push(p.clone()),
+            _ => {}
+        }
+    }
+    assert!(checked > 3000, "only checked {checked} group files");
+    assert!(
+        empty.is_empty(),
+        "{} of {checked} group files parsed to an empty group: {:?}",
+        empty.len(),
+        &empty[..empty.len().min(10)]
+    );
+}

crates/benilla-wmo/src/lib.rs

@@ -164,17 +164,37 @@ type Result<T> = std::result::Result<T, Error>;
 pub fn parse_wmo(cursor: &mut Cursor<&[u8]>) -> Result<ParsedWmo> {
     let b: &[u8] = cursor.get_ref();
     // A group file carries a MOGP super-chunk; a root carries MOHD.
-    for (magic, payload) in chunks(b) {
-        if &magic == b"PGOM" {
-            return Ok(ParsedWmo::Group(parse_group(payload)?));
-        }
-        if &magic == b"DHOM" {
-            return Ok(ParsedWmo::Root(parse_root(b)?));
-        }
+    if let Some(payload) = mogp_payload(b) {
+        return Ok(ParsedWmo::Group(parse_group(payload)?));
+    }
+    if chunks(b).any(|(magic, _)| &magic == b"DHOM") {
+        return Ok(ParsedWmo::Root(parse_root(b)?));
     }
     Err(Error::NotWmo)
 }
 
+/// The MOGP (`PGOM` as stored) payload of a group file: from its data start to **EOF**, whatever
+/// size it declares. MOGP is always a group file's last top-level chunk, so EOF is its real end,
+/// and a declared size shorter than the payload leaves the geometry sub-chunks as top-level
+/// siblings after it — where `chunks` would walk straight past them and [`parse_group`] would
+/// answer an empty group, a building that draws nothing.
+///
+/// This realm's chain has seven: the six `World\wmo\Outland\Shadowmoon\Slagpit\
+/// Shadowmoon_Slagpit01_00{0..5}.wmo` declare 0x40, short of the 0x44 header, and
+/// `HumanHouse\WestfallHouse_MediumStraw_000.wmo` declares 0.
+pub fn mogp_payload(b: &[u8]) -> Option<&[u8]> {
+    let mut pos = 0usize;
+    while pos.checked_add(8)? <= b.len() {
+        let size = b.u32_at(pos + 4)? as usize;
+        let start = pos + 8;
+        if &b[pos..pos + 4] == b"PGOM" {
+            return Some(&b[start..]);
+        }
+        pos = start.saturating_add(size).min(b.len());
+    }
+    None
+}
+
 fn parse_root(b: &[u8]) -> Result<WmoRoot> {
     let mut root = WmoRoot {
         textures: Vec::new(),
@@ -485,6 +505,31 @@ mod tests {
         assert!(g.liquid.is_none());
     }
 
+    #[test]
+    fn mogp_declaring_less_than_its_header_still_owns_the_sibling_chunks() {
+        // Shadowmoon_Slagpit01_00x: MOGP declares 0x40 (< the 0x44 header) and MOVT/MOVI/MOPY
+        // follow as top-level siblings up to EOF.
+        let mut hdr = vec![0u8; 68];
+        hdr[8..12].copy_from_slice(&0x48u32.to_le_bytes()); // flags
+        let mut b = chunk(b"REVM", &17u32.to_le_bytes());
+        b.extend_from_slice(b"PGOM");
+        b.extend_from_slice(&64u32.to_le_bytes());
+        b.extend_from_slice(&hdr);
+        let mut vert = Vec::new();
+        for f in [0f32, 0., 0., 1., 0., 0., 0., 1., 0.] {
+            vert.extend_from_slice(&f.to_le_bytes());
+        }
+        b.extend(chunk(b"TVOM", &vert));
+        b.extend(chunk(b"IVOM", &[0u8, 0, 1, 0, 2, 0]));
+        b.extend(chunk(b"YPOM", &[0u8, 0]));
+        let Ok(ParsedWmo::Group(g)) = parse(&b) else {
+            panic!("expected a group");
+        };
+        assert_eq!(g.flags, 0x48);
+        assert_eq!(g.vertex_positions.len(), 3);
+        assert_eq!(g.vertex_indices.len(), 3);
+    }
+
     #[test]
     fn truncated_mohd_is_an_error_not_a_panic() {
         // A 3-byte MOHD payload cannot hold nGroups at 4.