Strict mode object literal validation

src/parser.cc

 // Copyright 2010 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 2994 matching lines @@
 Handle<Object> Parser::GetBoilerplateValue(Expression* expression) {
   if (expression->AsLiteral() != NULL) {
     return expression->AsLiteral()->handle();
   }
   if (CompileTimeValue::IsCompileTimeValue(expression)) {
     return CompileTimeValue::GetValue(expression);
   }
   return Factory::undefined_value();
 }
 
+// Defined in ast.cc
+bool IsEqualString(void* first, void* second);

[Martin Maly, 2011/01/21 00:41:06]

These 2 are defined in ast.cc, ideally I'd want to move them to some shared
location. Is there a good place? This declaration feels suboptimal.

+bool IsEqualSmi(void* first, void* second);
+
+
+// Validation per 11.1.5 Object Initialiser
+class ObjectLiteralPropertyChecker {
+ public:
+  ObjectLiteralPropertyChecker(Parser* parser, bool strict) :
+    props(&IsEqualString),
+    elems(&IsEqualSmi),
+    parser_(parser),
+    strict_(strict) {
+  }
+
+  void CheckProperty(
+    ObjectLiteral::Property* property,
+    Scanner::Location loc,
+    bool* ok);
+
+ private:
+  enum PropertyKind {
+    GetAccessor = 0x01,

[Lasse Reichstein, 2011/01/21 12:01:13]

Add a "k" in front of constants, i.e., kGetAccessor, kSetAccessor, kAccessor,
kData.

[Martin Maly, 2011/01/21 20:00:37]

Done.

+    SetAccessor = 0x02,
+    Accessor = GetAccessor | SetAccessor,
+    Data = 0x04
+  };
+
+  HashMap props;

[Lasse Reichstein, 2011/01/21 12:01:13]

Properties should be at the end of the class, after methods (including static
methods).

[Martin Maly, 2011/01/21 20:00:37]

Done.

+  HashMap elems;
+  Parser* parser_;
+  bool strict_;
+
+  static intptr_t GetPropertyKind(ObjectLiteral::Property* property) {
+    switch (property->kind()) {
+      default:

[Lasse Reichstein, 2011/01/21 12:01:13]

Move the default to the end instead.
I just think it reads better :)

[Martin Maly, 2011/01/21 20:00:37]

Done.

+        return Data;
+      case ObjectLiteral::Property::GETTER:
+        return GetAccessor;
+      case ObjectLiteral::Property::SETTER:
+        return SetAccessor;
+    }
+  }
+};
+
+
+void ObjectLiteralPropertyChecker::CheckProperty(

[Martin Maly, 2011/01/21 00:41:06]

Similar code exists in ast.cc and is executed when emitting object literal. This
is too late because the object literal errors must be early errors.

+    ObjectLiteral::Property* property,
+    Scanner::Location loc,
+    bool* ok) {
+
+  ASSERT(property != NULL);
+
+  Literal *lit = property->key();
+  Handle<Object> handle = lit->handle();
+
+  uint32_t hash;
+  HashMap* map;
+  void* key;
+
+  if (handle->IsSymbol()) {
+    Handle<String> name(String::cast(*handle));
+    ASSERT(!name->AsArrayIndex(&hash));

[Lasse Reichstein, 2011/01/21 12:01:13]

I'm a little worried that we might have a symbol that is also a valid
ArrayIndex.
We might not, but I haven't been able to convince myself that it can't happen
yet.

[Martin Maly, 2011/01/21 20:00:37]

I am less concerned since the same checks are performed in ast.cc 
ObjectLiteral::CalculateEmitStore which has been around for a while.

[Lasse Reichstein, 2011/01/24 07:57:01]

Damnit. I fixed one such place already, but it seems there are more. It's good
that you found it :)
It's NOT safe. The ASSERT gets hit by, e.g.:
var y = {get "1234"() {return 42; }};

[Martin Maly, 2011/01/24 18:09:09]

Great catch. I fixed both locations and added more tests.

+    key = handle.location();
+    hash = name->Hash();
+    map = &props;
+  } else if (handle->ToArrayIndex(&hash)) {
+    key = handle.location();
+    map = &elems;
+  } else {
+    ASSERT(handle->IsNumber());
+    double num = handle->Number();
+    char arr[100];
+    Vector<char> buffer(arr, ARRAY_SIZE(arr));
+    const char* str = DoubleToCString(num, buffer);
+    Handle<String> name = Factory::NewStringFromAscii(CStrVector(str));
+    key = name.location();
+    hash = name->Hash();
+    map = &props;
+  }
+
+  // Lookup property previously defined, if any.
+  HashMap::Entry* entry = map->Lookup(key, hash, true);
+  intptr_t prev = reinterpret_cast<intptr_t> (entry->value);
+  intptr_t curr = GetPropertyKind(property);
+
+  // Duplicate data properties are illegal in strict mode.
+  if (strict_ && (curr & prev & Data) != 0) {
+    parser_->ReportMessageAt(loc, "strict_duplicate_property",
+                            Vector<const char*>::empty());
+    *ok = false;
+    return;
+  }
+  // Data property conflicting with an accessor.

[Martin Maly, 2011/01/21 00:41:06]

Most of these checks should execute even outside of strict mode. Only duplicate
data properties are strict mode check.

[Lasse Reichstein, 2011/01/21 12:01:13]

We should be careful when adding restrictions to non-strict code that isn't
there already. There is a potential to break existing code on the web.

We should at least check whether other significant browsers already fail on the
code, and not be the first to break the code.

[MarkM, 2011/01/21 17:37:55]

On WebKit nightly Version 5.0.1 (5533.17.8, r75891) non-strict

    ({x: 33, get x(){}});  
    SyntaxError: Parse error

On Minefield/FF 4.0b10pre (2011-01-21) non-strict

    ({x: 33, get x(){}});
    SyntaxError: property name x appears more than once in object literal

[Martin Maly, 2011/01/21 20:00:37]

Thank you, Mark, for doing the research. The shipping versions of Firefox,
Safari do not report error, however. Since their future versions are
implementing this I would recommend we keep the full checks in place as
implemented, although there may be good reason (like fast Chrome release cycle)
that may lead us to wait with full check until some future date and leave them
all under strict mode for now.

+  if (((curr & Data) && (prev & Accessor)) ||
+      ((prev & Data) && (curr & Accessor))) {
+    parser_->ReportMessageAt(loc, "accessor_data_property",
+                            Vector<const char*>::empty());
+    *ok = false;
+    return;
+  }
+  // Two accessors of the same type conflicting
+  if ((curr & prev & Accessor) != 0) {
+    parser_->ReportMessageAt(loc, "accessor_get_set",
+                            Vector<const char*>::empty());
+    *ok = false;
+    return;
+  }
+
+  // Update map
+  entry->value = reinterpret_cast<void*> (prev | curr);
+  *ok = true;
+}
+
 
 void Parser::BuildObjectLiteralConstantProperties(
     ZoneList<ObjectLiteral::Property*>* properties,
     Handle<FixedArray> constant_properties,
     bool* is_simple,
     bool* fast_elements,
     int* depth) {
   int position = 0;
   // Accumulate the value in local variables and store it at the end.
   bool is_simple_acc = true;
@@ skipping 85 matching lines @@
   // ObjectLiteral ::
   //   '{' (
   //       ((IdentifierName | String | Number) ':' AssignmentExpression)
   //     | (('get' | 'set') (IdentifierName | String | Number) FunctionLiteral)
   //    )*[','] '}'
 
   ZoneList<ObjectLiteral::Property*>* properties =
       new ZoneList<ObjectLiteral::Property*>(4);
   int number_of_boilerplate_properties = 0;
 
+  ObjectLiteralPropertyChecker checker(this, temp_scope_->StrictMode());
+
   Expect(Token::LBRACE, CHECK_OK);
+  Scanner::Location loc = scanner().location();
+
   while (peek() != Token::RBRACE) {
     if (fni_ != NULL) fni_->Enter();
 
     Literal* key = NULL;
     Token::Value next = peek();
+
+    // Location of the property name token
+    Scanner::Location loc = scanner().peek_location();
+
     switch (next) {
       case Token::IDENTIFIER: {
         bool is_getter = false;
         bool is_setter = false;
         Handle<String> id =
             ParseIdentifierOrGetOrSet(&is_getter, &is_setter, CHECK_OK);
         if (fni_ != NULL) fni_->PushLiteralName(id);
 
         if ((is_getter || is_setter) && peek() != Token::COLON) {
+            // Update loc to point to the identifier
+            loc = scanner().peek_location();
             ObjectLiteral::Property* property =
                 ParseObjectLiteralGetSet(is_getter, CHECK_OK);
             if (IsBoilerplateProperty(property)) {
               number_of_boilerplate_properties++;
             }
+            // Validate the property.
+            checker.CheckProperty(property, loc, CHECK_OK);
             properties->Add(property);
             if (peek() != Token::RBRACE) Expect(Token::COMMA, CHECK_OK);
 
             if (fni_ != NULL) {
               fni_->Infer();
               fni_->Leave();
             }
             continue;  // restart the while
         }
         // Failed to parse as get/set property, so it's just a property
@@ skipping 36 matching lines @@
     }
 
     Expect(Token::COLON, CHECK_OK);
     Expression* value = ParseAssignmentExpression(true, CHECK_OK);
 
     ObjectLiteral::Property* property =
         new ObjectLiteral::Property(key, value);
 
     // Count CONSTANT or COMPUTED properties to maintain the enumeration order.
     if (IsBoilerplateProperty(property)) number_of_boilerplate_properties++;
+    // Validate the property
+    checker.CheckProperty(property, loc, CHECK_OK);
     properties->Add(property);
 
     // TODO(1240767): Consider allowing trailing comma.
     if (peek() != Token::RBRACE) Expect(Token::COMMA, CHECK_OK);
 
     if (fni_ != NULL) {
       fni_->Infer();
       fni_->Leave();
     }
   }
   Expect(Token::RBRACE, CHECK_OK);
+
   // Computation of literal_index must happen before pre parse bailout.
   int literal_index = temp_scope_->NextMaterializedLiteralIndex();
 
   Handle<FixedArray> constant_properties =
       Factory::NewFixedArray(number_of_boilerplate_properties * 2, TENURED);
 
   bool is_simple = true;
   bool fast_elements = true;
   int depth = 1;
   BuildObjectLiteralConstantProperties(properties,
@@ skipping 1601 matching lines @@
       Handle<String> source = Handle<String>(String::cast(script->source()));
       result = parser.ParseProgram(source, info->is_global());
     }
   }
 
   info->SetFunction(result);
   return (result != NULL);
 }
 
 } }  // namespace v8::internal