From b652d7a4c52e38694d95dca3d857779288ee471f Mon Sep 17 00:00:00 2001
From: Mark Williams <mwilliams@fb.com>
Date: Tue, 25 Jun 2019 07:30:27 -0700
Subject: [PATCH] Fix AttrNoOverride* on classes

Summary:
We weren't checking for uniqueness before setting the magic method
no-override flags, which could in theory result in setting them
incorrectly.

eg if there's a single class B which extends A, with two definitions
of A, and a class C which defines a magic getter, and extends B, but
which is incompatible with one of the As, we'll incorrectly set the
no-override bit on B because *one* possible instantiation of B can't
have a magic getter.

I couldn't come up with code that behaves incorrectly, but

  <?hh
  class A {};
  class B extends A {};
  class C extends B {
    const FOO = 42;
    public function __get($n) { echo "getting $n\n"; return 42; }
  }
  function f(B $b) {
    return $b->foo;
  }
  <<__EntryPoint>>
  function main() {
    if (false) {
      include "bug.inc";
    }
    var_dump(f(new C));
  }

with bug.inc:

  <?hh
  class A {
    const type FOO = int;
  };

Gets the wrong flags on B without this diff.

Reviewed By: ricklavoie, alexeyt

Differential Revision: D15966816

fbshipit-source-id: 4b4a94e7a5107213377849d9e8ae5d9d1f32105e
---
 hphp/hhbbc/index.cpp | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/hphp/hhbbc/index.cpp b/hphp/hhbbc/index.cpp
index d5282913380..7caccc8b1dc 100644
--- a/hphp/hhbbc/index.cpp
+++ b/hphp/hhbbc/index.cpp
@@ -1774,6 +1774,14 @@ void add_unit_to_index(IndexData& index, const php::Unit& unit) {
   > closureMap;
 
   for (auto& c : unit.classes) {
+    auto const attrsToRemove =
+      AttrNoOverride |
+      AttrNoOverrideMagicGet |
+      AttrNoOverrideMagicSet |
+      AttrNoOverrideMagicIsset |
+      AttrNoOverrideMagicUnset;
+    attribute_setter(c->attrs, false, attrsToRemove);
+
     if (c->attrs & AttrEnum) {
       index.enums.insert({c->name, c.get()});
     }
@@ -2946,14 +2954,13 @@ void find_mocked_classes(IndexData& index) {
 
 void mark_no_override_classes(IndexData& index) {
   for (auto& cinfo : index.allClassInfos) {
-    auto const set = [&] {
-      if (!(cinfo->cls->attrs & AttrUnique) ||
-          cinfo->cls->attrs & AttrInterface) {
-        return false;
-      }
-      return cinfo->subclassList.size() == 1;
-    }();
-    attribute_setter(cinfo->cls->attrs, set, AttrNoOverride);
+    // We cleared all the NoOverride flags while building the
+    // index. Set them as necessary.
+    if (!(cinfo->cls->attrs & AttrUnique)) continue;
+    if (!(cinfo->cls->attrs & AttrInterface) &&
+        cinfo->subclassList.size() == 1) {
+      attribute_setter(cinfo->cls->attrs, true, AttrNoOverride);
+    }
 
     for (auto& kv : magicMethodMap) {
       if (kv.second.attrBit == AttrNone) continue;
-- 
2.11.4.GIT