diff --git a/rslib/src/notetype/merge.rs b/rslib/src/notetype/merge.rs index 83c5871c8ac..3860b8df904 100644 --- a/rslib/src/notetype/merge.rs +++ b/rslib/src/notetype/merge.rs @@ -25,11 +25,20 @@ impl Notetype { for (index, field) in other.fields.iter().enumerate() { match self.find_field(field) { Some(i) if i == index => (), - Some(i) => self.fields.swap(i, index), + // `other` may have more fields than us (e.g. a corrupt notetype + // with a field matching one of ours twice). Only re-align when + // `index` is a real slot in our list; otherwise the field is + // already present, so leave it in place rather than panicking. + Some(i) => { + if index < self.fields.len() { + self.fields.swap(i, index); + } + } None => { let mut missing = field.clone(); missing.ord.take(); - self.fields.insert(index, missing); + // clamp so a longer `other` appends instead of panicking + self.fields.insert(index.min(self.fields.len()), missing); } } } @@ -47,11 +56,18 @@ impl Notetype { for (index, template) in other.templates.iter().enumerate() { match self.find_template(template) { Some(i) if i == index => (), - Some(i) => self.templates.swap(i, index), + // see merge_fields: only re-align when `index` is a real slot; + // a longer `other` would otherwise go out of bounds + Some(i) => { + if index < self.templates.len() { + self.templates.swap(i, index); + } + } None => { let mut missing = template.clone(); missing.ord.take(); - self.templates.insert(index, missing); + self.templates + .insert(index.min(self.templates.len()), missing); } } } @@ -91,6 +107,8 @@ impl CardTemplate { #[cfg(test)] mod test { + use std::collections::HashSet; + use itertools::assert_equal; use super::*; @@ -160,6 +178,47 @@ mod test { assert_equal(basic.template_names(), std::iter::once("Card 1")); } + #[test] + fn merge_longer_other_with_duplicate_field() { + // A malformed incoming notetype (e.g. from a corrupt deck) can have more + // fields than ours, including a field that matches one of ours a second + // time. Merging it must not panic. See issue #4345. + let mut basic = stock::basic(&I18n::template_only()); + let mut other = basic.clone(); + other.add_field("Extra1"); + other.add_field("Extra2"); + // duplicate the first field so it matches `basic`'s first field again, + // at an index beyond `basic`'s length + let duplicate = other.fields[0].clone(); + other.fields.push(duplicate); + + basic.merge(&other); + + // every distinct incoming field is present, with no duplicates + let names: HashSet<&str> = basic.field_names().map(String::as_str).collect(); + assert_eq!(names, HashSet::from(["Front", "Back", "Extra1", "Extra2"])); + assert_eq!(basic.fields.len(), 4); + } + + #[test] + fn merge_longer_other_with_duplicate_template() { + let mut basic = stock::basic_forward_reverse(&I18n::template_only()); + let mut other = basic.clone(); + other.add_template("Extra1", "", ""); + other.add_template("Extra2", "", ""); + let duplicate = other.templates[0].clone(); + other.templates.push(duplicate); + + basic.merge(&other); + + let names: HashSet<&str> = basic.template_names().map(String::as_str).collect(); + assert_eq!( + names, + HashSet::from(["Card 1", "Card 2", "Extra1", "Extra2"]) + ); + assert_eq!(basic.templates.len(), 4); + } + #[test] fn align_template_order() { let mut basic_rev = stock::basic_forward_reverse(&I18n::template_only());