From fd2f3e7b404a9998e9cfe8a942237bb27d5b1775 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Thu, 31 Aug 2023 13:19:58 -0400 Subject: [PATCH 1/2] [write-fonts] Add more validation logic for PairPos2 This ensures that the arrays are all the correct shape, and that all of the value records report the expected format. I believe we are getting these things wrong in fontc/fea-rs, in some cases. --- font-codegen/README.md | 3 + resources/codegen_inputs/gpos.rs | 1 + write-fonts/generated/generated_gpos.rs | 1 + write-fonts/src/tables/gpos.rs | 106 ++++++++++++++++++++++++ write-fonts/src/tables/layout.rs | 6 ++ write-fonts/src/tables/value_record.rs | 34 +------- 6 files changed, 119 insertions(+), 32 deletions(-) diff --git a/font-codegen/README.md b/font-codegen/README.md index 584a2d95e..733aa3883 100644 --- a/font-codegen/README.md +++ b/font-codegen/README.md @@ -159,6 +159,9 @@ The following annotations are supported on top-level objects: between GPOS and GSUB. - `#[write_fonts_only]` Indicate that this table should only be generated for `write-fonts` (i.e. should be ignored in `read-fonts`). +- `#[validate(method)]` Provide a method to perform additional pre-compilation + validation for this type. The method must be manually implemented on the type, + with the signature `fn(&self, &mut ValidationCtx)`. #### field attributes - `#[nullable]`: only allowed on offsets or arrays of offsets, and indicates diff --git a/resources/codegen_inputs/gpos.rs b/resources/codegen_inputs/gpos.rs index c70c40e96..f289af6d4 100644 --- a/resources/codegen_inputs/gpos.rs +++ b/resources/codegen_inputs/gpos.rs @@ -232,6 +232,7 @@ record PairValueRecord { } /// [Pair Adjustment Positioning Format 2](https://docs.microsoft.com/en-us/typography/opentype/spec/gpos#pair-adjustment-positioning-format-2-class-pair-adjustment): Class Pair Adjustment +#[validate(check_length_and_format_conformance)] table PairPosFormat2 { /// Format identifier: format = 2 #[format = 2] diff --git a/write-fonts/generated/generated_gpos.rs b/write-fonts/generated/generated_gpos.rs index 2015df07b..9cd5a453b 100644 --- a/write-fonts/generated/generated_gpos.rs +++ b/write-fonts/generated/generated_gpos.rs @@ -1154,6 +1154,7 @@ impl Validate for PairPosFormat2 { } self.class1_records.validate_impl(ctx); }); + self.check_length_and_format_conformance(ctx); }) } } diff --git a/write-fonts/src/tables/gpos.rs b/write-fonts/src/tables/gpos.rs index 11ddd4fb4..838143a00 100644 --- a/write-fonts/src/tables/gpos.rs +++ b/write-fonts/src/tables/gpos.rs @@ -133,6 +133,35 @@ impl PairPosFormat2 { fn compute_class2_count(&self) -> u16 { self.class_def2.class_count() } + + fn check_length_and_format_conformance(&self, ctx: &mut ValidationCtx) { + let n_class_1s = self.class_def1.class_count(); + let n_class_2s = self.class_def2.class_count(); + let format_1 = self.compute_value_format1(); + let format_2 = self.compute_value_format2(); + if self.class1_records.len() != n_class_1s as usize { + ctx.report("class1_records length must match number of class1 classes"); + } + ctx.in_field("class1_records", |ctx| { + ctx.in_array(|ctx| { + for record in &self.class1_records { + ctx.array_item(|ctx| { + if record.class2_records.len() != n_class_2s as usize { + ctx.report( + "class2_records length must match number of class2 classes ", + ); + } + if record.class2_records.iter().any(|rec| { + rec.value_record1.format() != format_1 + || rec.value_record2.format() != format_2 + }) { + ctx.report("all value records should report the same format"); + } + }) + } + }) + }); + } } impl MarkBasePosFormat1 { @@ -206,4 +235,81 @@ mod tests { } ); } + + // shared between a pair of tests below + fn make_rec(i: u16) -> ValueRecord { + // '0' here is shorthand for 'no device table' + if i == 0 { + return ValueRecord::new().with_explicit_value_format(ValueFormat::X_ADVANCE_DEVICE); + } + ValueRecord::new().with_x_advance_device(VariationIndex::new(0xff, i)) + } + + #[test] + fn compile_devices_pairpos2() { + let class1 = ClassDef::from_iter([(GlyphId::new(5), 0), (GlyphId::new(6), 1)]); + // class 0 is 'all the rest', here, always implicitly present + let class2 = ClassDef::from_iter([(GlyphId::new(8), 1)]); + + // two c1recs, each with two c2recs + let class1recs = vec![ + Class1Record::new(vec![ + Class2Record::new(make_rec(0), make_rec(0)), + Class2Record::new(make_rec(1), make_rec(2)), + ]), + Class1Record::new(vec![ + Class2Record::new(make_rec(0), make_rec(0)), + Class2Record::new(make_rec(2), make_rec(3)), + ]), + ]; + let coverage = class1.iter().map(|(gid, _)| gid).collect(); + let a_table = PairPos::format_2(coverage, class1, class2, class1recs); + + let bytes = crate::dump_table(&a_table).unwrap(); + let read_back = PairPosFormat2::read(bytes.as_slice().into()).unwrap(); + + assert!(read_back.class1_records[0].class2_records[0] + .value_record1 + .x_advance_device + .is_none()); + assert!(read_back.class1_records[1].class2_records[1] + .value_record1 + .x_advance_device + .is_some()); + + let DeviceOrVariationIndex::VariationIndex(dev2) = read_back.class1_records[0] + .class2_records[1] + .value_record2 + .x_advance_device + .as_ref() + .unwrap() + else { + panic!("not a variation index") + }; + assert_eq!(dev2.delta_set_inner_index, 2); + } + + #[should_panic(expected = "all value records should report the same format")] + #[test] + fn validate_bad_pairpos2() { + let class1 = ClassDef::from_iter([(GlyphId::new(5), 0), (GlyphId::new(6), 1)]); + // class 0 is 'all the rest', here, always implicitly present + let class2 = ClassDef::from_iter([(GlyphId::new(8), 1)]); + let coverage = class1.iter().map(|(gid, _)| gid).collect(); + + // two c1recs, each with two c2recs + let class1recs = vec![ + Class1Record::new(vec![ + Class2Record::new(make_rec(0), make_rec(0)), + Class2Record::new(make_rec(1), make_rec(2)), + ]), + Class1Record::new(vec![ + Class2Record::new(make_rec(0), make_rec(0)), + // this is now the wrong type + Class2Record::new(make_rec(2), make_rec(3).with_x_advance(0x514)), + ]), + ]; + let ppf2 = PairPos::format_2(coverage, class1, class2, class1recs); + crate::dump_table(&ppf2).unwrap(); + } } diff --git a/write-fonts/src/tables/layout.rs b/write-fonts/src/tables/layout.rs index c559673d9..279e5c8b0 100644 --- a/write-fonts/src/tables/layout.rs +++ b/write-fonts/src/tables/layout.rs @@ -454,6 +454,12 @@ impl FromIterator<(GlyphId, u16)> for ClassDefBuilder { } } +impl FromIterator<(GlyphId, u16)> for ClassDef { + fn from_iter>(iter: T) -> Self { + ClassDefBuilder::from_iter(iter).build() + } +} + impl ClassDefBuilder { fn prefer_format_1(&self) -> bool { // calculate our format2 size: diff --git a/write-fonts/src/tables/value_record.rs b/write-fonts/src/tables/value_record.rs index 8dc83f8c8..b290432b3 100644 --- a/write-fonts/src/tables/value_record.rs +++ b/write-fonts/src/tables/value_record.rs @@ -227,8 +227,8 @@ mod tests { use read_fonts::FontRead; use crate::tables::{ - gpos::{Class1Record, Class2Record, PairPos, PairPosFormat2, SinglePos, SinglePosFormat1}, - layout::{ClassDefBuilder, CoverageTableBuilder, VariationIndex}, + gpos::{SinglePos, SinglePosFormat1}, + layout::{CoverageTableBuilder, VariationIndex}, }; use super::*; @@ -261,34 +261,4 @@ mod tests { matches!(read_back.value_record.x_advance_device.as_ref(), Some(DeviceOrVariationIndex::VariationIndex(var_idx)) if var_idx.delta_set_inner_index == 0xee) ) } - - #[test] - fn compile_devices_pairpos2() { - let rec1 = ValueRecord::new().with_x_advance_device(VariationIndex::new(0xff, 0xee)); - let rec2 = ValueRecord::new().with_x_advance_device(VariationIndex::new(0xaa, 0xbb)); - let class1 = - ClassDefBuilder::from_iter([(GlyphId::new(5), 2), (GlyphId::new(6), 2)]).build(); - let class2 = - ClassDefBuilder::from_iter([(GlyphId::new(8), 3), (GlyphId::new(9), 3)]).build(); - let class1recs = vec![Class1Record::new(vec![Class2Record::new(rec1, rec2)])]; - let a_table = PairPos::format_2( - CoverageTableBuilder::from_glyphs(vec![GlyphId::new(42)]).build(), - class1, - class2, - class1recs, - ); - - let bytes = crate::dump_table(&a_table).unwrap(); - let read_back = PairPosFormat2::read(bytes.as_slice().into()).unwrap(); - let DeviceOrVariationIndex::VariationIndex(dev2) = read_back.class1_records[0] - .class2_records[0] - .value_record2 - .x_advance_device - .as_ref() - .unwrap() - else { - panic!("not a variation index") - }; - assert_eq!(dev2.delta_set_outer_index, 0xaa); - } } From 6ae450cff0971c8a0d4f380401cc59664c92267d Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Thu, 31 Aug 2023 14:00:18 -0400 Subject: [PATCH 2/2] [write-fonts] Additional validation for PairPos1 Another check to ensure that all value records report the correct format. --- resources/codegen_inputs/gpos.rs | 1 + write-fonts/generated/generated_gpos.rs | 2 +- write-fonts/src/tables/gpos.rs | 66 +++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 1 deletion(-) diff --git a/resources/codegen_inputs/gpos.rs b/resources/codegen_inputs/gpos.rs index f289af6d4..43bfb6378 100644 --- a/resources/codegen_inputs/gpos.rs +++ b/resources/codegen_inputs/gpos.rs @@ -201,6 +201,7 @@ table PairPosFormat1 { /// of PairPos subtable, ordered by Coverage Index. #[count($pair_set_count)] #[read_offset_with($value_format1, $value_format2)] + #[validate(check_format_consistency)] pair_set_offsets: [Offset16], } diff --git a/write-fonts/generated/generated_gpos.rs b/write-fonts/generated/generated_gpos.rs index 9cd5a453b..110f8d2e8 100644 --- a/write-fonts/generated/generated_gpos.rs +++ b/write-fonts/generated/generated_gpos.rs @@ -953,7 +953,7 @@ impl Validate for PairPosFormat1 { if self.pair_sets.len() > (u16::MAX as usize) { ctx.report("array exceeds max length"); } - self.pair_sets.validate_impl(ctx); + self.check_format_consistency(ctx); }); }) } diff --git a/write-fonts/src/tables/gpos.rs b/write-fonts/src/tables/gpos.rs index 838143a00..715160d91 100644 --- a/write-fonts/src/tables/gpos.rs +++ b/write-fonts/src/tables/gpos.rs @@ -107,6 +107,27 @@ impl PairPosFormat1 { .map(|rec| rec.value_record2.format()) .unwrap_or(ValueFormat::empty()) } + + fn check_format_consistency(&self, ctx: &mut ValidationCtx) { + let vf1 = self.compute_value_format1(); + let vf2 = self.compute_value_format2(); + ctx.in_array(|ctx| { + for pairset in &self.pair_sets { + ctx.array_item(|ctx| { + ctx.in_field("pair_value_records", |ctx| { + ctx.in_array(|ctx| { + if pairset.pair_value_records.iter().any(|pairset| { + pairset.value_record1.format() != vf1 + || pairset.value_record2.format() != vf2 + }) { + ctx.report("all ValueRecords must have same format") + } + }) + }) + }) + } + }) + } } impl PairPosFormat2 { @@ -197,6 +218,8 @@ mod tests { use read_fonts::tables::{gpos as read_gpos, layout::LookupFlag}; + use crate::tables::layout::VariationIndex; + use super::*; // adapted from/motivated by https://github.com/fonttools/fonttools/issues/471 @@ -312,4 +335,47 @@ mod tests { let ppf2 = PairPos::format_2(coverage, class1, class2, class1recs); crate::dump_table(&ppf2).unwrap(); } + + #[test] + fn validate_pairpos1() { + let coverage: CoverageTable = [1, 2].into_iter().map(GlyphId::new).collect(); + let good_table = PairPosFormat1::new( + coverage.clone(), + vec![ + PairSet::new(vec![PairValueRecord::new( + GlyphId::new(5), + ValueRecord::new().with_x_advance(5), + ValueRecord::new(), + )]), + PairSet::new(vec![PairValueRecord::new( + GlyphId::new(1), + ValueRecord::new().with_x_advance(42), + ValueRecord::new(), + )]), + ], + ); + + let bad_table = PairPosFormat1::new( + coverage, + vec![ + PairSet::new(vec![PairValueRecord::new( + GlyphId::new(5), + ValueRecord::new().with_x_advance(5), + ValueRecord::new(), + )]), + PairSet::new(vec![PairValueRecord::new( + GlyphId::new(1), + //this is a different format, which is not okay + ValueRecord::new().with_x_placement(42), + ValueRecord::new(), + )]), + ], + ); + + assert!(crate::dump_table(&good_table).is_ok()); + assert!(matches!( + crate::dump_table(&bad_table), + Err(crate::error::Error::ValidationFailed(_)) + )); + } }