Avoid undefined-behavior, another try

[behdad]

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1577584

This looks like should work to me; but multiple tests fail. Leaving here to be debugged. Maybe someone helps me. :)

[blueshade7]

Not sure what is the right fix, but I see why it’s failing.
blob->as<OT::OS2> () returns Null obj for version 3 OS/2 for example, because its size is smaller than null_size:

harfbuzz/src/hb-array.hh

Lines 196 to 197 in 66dfd60

	const T *as () const
	{ return length < hb_null_size (T) ? &Null (T) : reinterpret_cast<const T *> (arrayZ); }

null_size is set to sizeof (T) by the PR, in the case of OS/2, version 5 table, larger than version 3.

[ebraminio]

It is possible to workaround OS/2 issue by,

diff --git a/src/hb-ot-os2-table.hh b/src/hb-ot-os2-table.hh
index 04a21af1..d69e01bd 100644
--- a/src/hb-ot-os2-table.hh
+++ b/src/hb-ot-os2-table.hh
@@ -117,9 +117,9 @@ struct OS2
 
   bool has_data () const { return usWeightClass || usWidthClass || usFirstCharIndex || usLastCharIndex; }
 
-  const OS2V1Tail &v1 () const { return version >= 1 ? v1X : Null (OS2V1Tail); }
-  const OS2V2Tail &v2 () const { return version >= 2 ? v2X : Null (OS2V2Tail); }
-  const OS2V5Tail &v5 () const { return version >= 5 ? v5X : Null (OS2V5Tail); }
+  const OS2V1Tail &v1 () const { return version >= 1 ? StructAtOffset<OS2V1Tail> (this, min_size) : Null (OS2V1Tail); }
+  const OS2V2Tail &v2 () const { return version >= 2 ? StructAfter<OS2V2Tail> (v1 ()) : Null (OS2V2Tail); }
+  const OS2V5Tail &v5 () const { return version >= 5 ? StructAfter<OS2V5Tail> (v2 ()) : Null (OS2V5Tail); }
 
   enum selection_flag_t {
     ITALIC             = 1u<<0,
@@ -185,7 +185,7 @@ struct OS2
   void _update_unicode_ranges (const hb_set_t *codepoints,
                               HBUINT32 ulUnicodeRange[4]) const
   {
-    HBUINT32   newBits[4];
+    HBUINT32 newBits[4];
     for (unsigned int i = 0; i < 4; i++)
       newBits[i] = 0;
 
@@ -236,9 +236,9 @@ struct OS2
   unsigned get_size () const
   {
     unsigned result = min_size;
-    if (version >= 1) result += v1X.get_size ();
-    if (version >= 2) result += v2X.get_size ();
-    if (version >= 5) result += v5X.get_size ();
+    if (version >= 1) result += OS2V1Tail::static_size;
+    if (version >= 2) result += OS2V2Tail::static_size;
+    if (version >= 5) result += OS2V5Tail::static_size;
     return result;
   }
 
@@ -246,9 +246,9 @@ struct OS2
   {
     TRACE_SANITIZE (this);
     if (unlikely (!c->check_struct (this))) return_trace (false);
-    if (unlikely (version >= 1 && !v1X.sanitize (c))) return_trace (false);
-    if (unlikely (version >= 2 && !v2X.sanitize (c))) return_trace (false);
-    if (unlikely (version >= 5 && !v5X.sanitize (c))) return_trace (false);
+    if (unlikely (version >= 1 && !v1 ().sanitize (c))) return_trace (false);
+    if (unlikely (version >= 2 && !v2 ().sanitize (c))) return_trace (false);
+    if (unlikely (version >= 5 && !v5 ().sanitize (c))) return_trace (false);
     return_trace (true);
   }
 
@@ -280,9 +280,9 @@ struct OS2
   HBINT16      sTypoLineGap;
   HBUINT16     usWinAscent;
   HBUINT16     usWinDescent;
-  OS2V1Tail    v1X;
+/*OS2V1Tail    v1X;
   OS2V2Tail    v2X;
-  OS2V5Tail    v5X;
+  OS2V5Tail    v5X;*/
   public:
   DEFINE_SIZE_MIN (78);
 };

But apparently that, maybe, isn't in the right direction.

[behdad]

Hold on making changes please. I'll comment soon.

[behdad]

Okay! I have a fix here. I think is good. Will going to merge if bots are happy, and then re-evaluate use of flexible arrays separately.