701364 - hasOwnProperty throws "TypeError: can't convert undefined to object"

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Thomas Fischer]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0) Gecko/20100101 Firefox/8.0
Build ID: 20111104165243

Steps to reproduce:

hasOwnProperty("foo")


Actual results:

TypeError: can't convert undefined to object


Expected results:

it should return false
The function "hasOwnProperty" should be invoked in the context of the global object

Comment 1

[:aceman]

Can you point to the spec that defines this?

Comment 2

[Thomas Fischer]

Standard ECMA-262, Edition 5.1 (June 2011)

  10.4.3 Entering Function Code
    [...]
    2. Else if thisArg is null or undefined, set the ThisBinding
       to the global object.

    15.2.4.5 Object.prototype.hasOwnProperty (V)
      [...]
      2. Let O be the result of calling ToObject passing the this
         value as the argument.

I tested the following code also in differnt browsers:

  foo = "bar";
  hasOwnProperty("foo");

  MSIE 9:     true
  Chrome: 15  false
  Safari 5.1: false
  FF 7.01:    TypeError

Only FF throws an error, only IE works as expected

Comment 3

[Thomas Fischer]

BTW:
The bug concerns a lot of methods of Object.prototype

toString works fine, but valueOf etc. not

console.log(toString.call(null)); // [object Window]
foo = "bar";
console.log(hasOwnProperty.call(this, "foo")); // true
console.log(hasOwnProperty.call(null, "foo")); // ERROR

Comment 4

[Jeff Walden [:Waldo]]

10.4.3 Entering Function Code refers to the behavior which should occur when entering the code for a function defined by an expression or statement in user-defined source code.  It does not define what should happen when a function not defined by an expression or statement -- that is, a host function or one defined in ECMAScript itself -- is called.  Host functions define whatever semantics they want for handling null/undefined this.  Functions defined in ECMAScript do whatever the steps of their algorithm indicate.  Object.prototype.hasOwnProperty does ToObject(this) in step 2, and since ToObject throws for null or undefined, the result of the hasOwnProperty call should be a thrown TypeError.

Comment 5

[Thomas Fischer]

Hi,
You are right, chapter 10 is about user-defined source code.
But hasOwnProperty (plus other functions) claim that they are ECMA-Script functions:
  FF8.0:
  typeof hasOwnProperty       // "function"
  hasOwnProperty.constructor  // Function()

EITHER
  they shouldn't claim to be "normal ECMA functions"
OR
  they should FULL FILL the ECMA specification.

All other is something like "Wash me, but don't make me wet"

Comment 6

[Masatoshi Kimura [:emk]]

>  Chrome: 15  false
>  Safari 5.1: false
I saw Chrom 15 and Safari 5.1 throw TypeError. Opera Next 12 also throws.
>  typeof hasOwnProperty       // "function"
>  hasOwnProperty.constructor  // Function()
There's nothing wrong per spec.
>  they shouldn't claim to be "normal ECMA functions"
Define "normal ECMA functions".
>  they should FULL FILL the ECMA specification.
We need to throw TypeError to comply the spec.

Comment 7

[Thomas Fischer]

> I saw Chrom 15 and Safari 5.1 throw TypeError.
> Opera Next 12 also throws.
If you call it without dot-operator it works fine,
even with FF 3.6.19 Win:
  hasOwnProperty("foo")

> There's nothing wrong per spec.
"I'm a function but I don't care about ECMA specs" sounds very wrong for me.

> Define "normal ECMA functions".
  hasOwnProperty..constructor === Function // true
  hasOwnProperty.__proto__    === Function.prototype  // true
  hasOwnProperty.call === Function.call // true
  hasOwnProperty.apply === Function.prototype.apply // true
  hasOwnProperty.call === Function.prototype.call   // true

It seems there is no way to convince you that we talk about a bug or at least a inconsistency ... See you later aligator. After while crocodile.

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: testcase

> If you call it without dot-operator it works fine,
What does "works fine" mean? I'll attach a test case to clarify. Here's my result:
 Firefox 8: TypeError: can't convert undefined to object
 Chrome 15: TypeError: Cannot convert null to object
 Safari 5.1: TypeError: 'undefined' is not an object (evaluating 'hasOwnProperty("foo")')
 Opera Next 15: TypeError: Cannot convert undefined or null to Object

> "I'm a function but I don't care about ECMA specs" sounds very wrong for me.
What part of ECMA spec is not cared? Please cite form the spec.
(Actually, we throw TypeError because we care about the spec.)

>  hasOwnProperty..constructor === Function // true
>  hasOwnProperty.call === Function.call // true
>  hasOwnProperty.apply === Function.prototype.apply // true
>  hasOwnProperty.call === Function.prototype.call   // true
Then what contradicts throwing TypeError?

>  hasOwnProperty.__proto__    === Function.prototype  // true
__proto__ is not even defined in the spec. What are you talking about?

> It seems there is no way to convince you that we talk about a bug or at least a inconsistency ...
Because it is not a bug.

Comment 9

[Thomas Fischer]

Hi,
> What does "works fine" mean? [...]
Sorry my fault, I tried it out in browser consoles,
but the code in the console is executed with a different thisValue
then global object :-( 

> (Actually, we throw TypeError because we care about the spec.)
OK, now I found a hint in ECMAScript 5.1 which explains why FF8 stick to the spec:
   15.3.4.4  Function.prototype.call 
   [...]
   NOTE The thisArg value is passed without modification as the this value.
   This is a change from Edition 3, where a undefined or null thisArg is
   replaced with the global object and ToObject is applied to all other
   values and that result is passed as the this value.

Unfortunately this spec change will cause a lot of downward incompatibilities for existig code. IMHO this change should have happen in strict mode only.

Comment 10

[David Bruant]

(In reply to Thomas Fischer from comment #0)
> Steps to reproduce:
> 
> hasOwnProperty("foo")
When executing this code, the first thing that is attempted is to resolve "hasOwnProperty".
ES5.1 - 11.1.2, leading to 10.3.1. Here, the lexical environment is the global environment (defined in 10.2.3).
Step 3 of 10.3.1 leads to 10.2.2.1 GetIdentifierReference ((global environment), "hasOwnProperty", false).
Step 2 of 10.2.2.1 envRec is "The global environment’s Environment Record is an object environment record whose binding object is the global object" (10.2.3)
Call to HasBinding (10.2.1.2.1) which calls the [[HasProperty]] of the global object with "hasOwnProperty".

Here, the spec is silent on what the result should be. The spec doesn't say that the global object has an "hasOwnProperty" (own or inherited). Neither does it say specifically that the global object should inherit from Object.prototype (though it's what is done in current browsers).
It turns out SpiderMonkey makes the choice that the global object inherits from Object.prototoype, so it should answer "true" (which it does as far as I know).

Back to 10.2.2.1, exists is true, a Reference is returned with base value being the global environment Environment record, name being "hasOwnProperty" and strict being false.

The identifier resolution was part of a function call (11.2.3) (MemberExpression being a PrimaryExpression being an Identifier)
Step 2 of function call is GetValue(ref) (8.7.1) which goes through step 5 which, I'll make it short, should return the Object.prototype.hasOwnProperty function. This function will be called with undefined as |this| value.

> Actual results:
> 
> TypeError: can't convert undefined to object
as it should because it cannot convert the this value (undefined) into an object (step 2 of 15.2.4.5 Object.prototype.hasOwnProperty (V)).



(In reply to Thomas Fischer from comment #9)
> > (Actually, we throw TypeError because we care about the spec.)
> OK, now I found a hint in ECMAScript 5.1 which explains why FF8 stick to the
> spec:
>    15.3.4.4  Function.prototype.call 
The snippet you showed is "hasOwnProperty('foo')", not "hasOwnProperty.call(undefined, 'foo')", so that's irrelevant. The relevant part is 11.2.3 Function Calls.

> (...)
> Unfortunately this spec change will cause a lot of downward
> incompatibilities for existig code.
True and this has been made on purpose. It allowed any code to have access to the global object and consequently being allowed to mess with it which was a security flaw. It is likely that indeed, a bunch of malicious code got broken because of this change :-)

Comment 11

[Jeff Walden [:Waldo]]

You'd think there were a lot of incompatibilities, but there weren't.  We got one honest-to-goodness bug report about the change:

http://whereswalden.com/2011/02/03/working-on-the-js-engine-episode-iv/

As you can imagine, this was not a sufficiently strong use case to revert the change.

Moreover, as David notes, this change did break a fair bit of malicious code.  Back in the early days of Facebook's app platform, they had a fair number of different holes from this:

http://stuff.mit.edu/iap/2008/facebook/

Many to most of the holes demonstrated there were closed by this change, such that if you could run FF8 against Facebook circa 2007 with those holes in place, the attacks wouldn't work.

It's certainly possible to argue that calling it a "security flaw" is overblown.  In some sense the Facebook JS systems of the world are pretty far outliers to design for.  But for better or for worse the language got changed for this (I happen to think for the better, myself), and in more than half a year there have been very few problems.  So the change is going to stick.

It's worth noting that the web console is kind of screwy as far as execution goes, and not like just having an identical script in the page.  There are advantages and disadvantages to this, and I can't really say it's worse this way than some other less-screwy but more page-visible way.  Just keep in mind that the web console is not as similar to just executing in the page itself as you might hope.

Comment 12

[Dmitry Soshnikov]

First, we have to say, that the case is not standard. The spec says nothing about that the global object should inherit from Object.prototype. Moreover, for the next version of ES, the global environment is planned to be declarative, not object, and therefore later it will be obvious that it shouldn't inherit from Object.prototype.

Anyway, in current SpiderMonkey implementation (and perhaps in some others) the global object nevertheless inherits from Object.prototype. And therefore "hasOwnProperty" binding is resolved exactly there.

Later, at function activation, the |this| value of the context should be set (in non-strict mode) to the global object. Since (ES 5.1): in 11.2.3 step 6.b.i - ImplicitThisValue - for global is undefined. Then, in 10.4.3 - step 2 - "Else if thisArg is null or undefined, set the ThisBinding to the global object".

So it seems just a "bug". Well, how to say bug? -- Once again, the case isn't standard at all. Nothing is said about that the global should inherit from Object.prototype. But if it does, then it's bug.

Dmitry.

P.S.: I even see the bug fixed (?) in SpiderMonkey 1.8.5

Comment 13

[Thomas Fischer]

Hi David & Jeff,
many thanks for your very detailed answers!

> Moreover, as David notes, this change did break a fair bit of malicious code

I can't see how this change should prevent malicious code to have access the global object, even if the function is sandboxed. I expect to have always access to the global object by doing:

  function malicious(){
    globalObject = (function(){return this})();
    // doing bad things
  }

salute
Thomas

Comment 14

[Dmitry Soshnikov]

(In reply to Jeff Walden (remove +bmo to email) from comment #4)
> 10.4.3 Entering Function Code refers to the behavior which should occur when
> entering the code for a function defined by an expression or statement in
> user-defined source code.  It does not define what should happen when a
> function not defined by an expression or statement -- that is, a host
> function or one defined in ECMAScript itself -- is called.  Host functions
> define whatever semantics they want for handling null/undefined this. 
> Functions defined in ECMAScript do whatever the steps of their algorithm
> indicate. 

What a nonsense.

> Object.prototype.hasOwnProperty does ToObject(this) in step 2,
> and since ToObject throws for null or undefined, the result of the
> hasOwnProperty call should be a thrown TypeError.

And this is simply wrong.

Dmitry.

Comment 15

[Jeff Walden [:Waldo]]

(In reply to Thomas Fischer from comment #13)
> I can't see how this change should prevent malicious code to have access the
> global object, even if the function is sandboxed. I expect to have always
> access to the global object by doing:
> 
>   function malicious(){
>     globalObject = (function(){return this})();
>     // doing bad things
>   }

The expectation is that such run-user-code platforms will require that all user-contributed code be strict mode code, such that this isn't possible.  It's also expected that these platforms will remove access to runtime code generation facilities like eval and Function (deleting both from the global, and deleting Function.prototype.constructor), such that user code can't break out of the strict mode prison, or out of the global environment as censored by those platforms.  I'm not aware of a comprehensive guide that documents these restrictions, unfortunately, to direct you toward.

(In reply to Dmitry A. Soshnikov from comment #14)
> And this is simply wrong.

I'm sorry, but I have to disagree.

Comment 16

[Jeff Walden [:Waldo]]

Alternatively, as the original (maybe still?) Facebook platform did, rewriting user code to eliminate references to Function and eval would also preserve safety.  The point of the ES5 changes was to eliminate the need to rewrite, given a platform that made the right changes to the initial environment in which user code would run.  But it also addressed these other holes, partly by happenstance, partly by design.

Comment 17

[Dmitry Soshnikov]

(In reply to Jeff Walden (remove +bmo to email) from comment #15)
> (In reply to Thomas Fischer from comment #13)
> > I can't see how this change should prevent malicious code to have access the
> > global object, even if the function is sandboxed. I expect to have always
> > access to the global object by doing:
> > 
> >   function malicious(){
> >     globalObject = (function(){return this})();
> >     // doing bad things
> >   }
> 
> The expectation is that such run-user-code platforms will require that all
> user-contributed code be strict mode code, such that this isn't possible. 
> It's also expected that these platforms will remove access to runtime code
> generation facilities like eval and Function (deleting both from the global,
> and deleting Function.prototype.constructor), such that user code can't
> break out of the strict mode prison, or out of the global environment as
> censored by those platforms.  I'm not aware of a comprehensive guide that
> documents these restrictions, unfortunately, to direct you toward.
> 

Though, even begin in (inherited from outer scopes) strict mode, there are still cases when indirect `eval' may create global bindings.

> (In reply to Dmitry A. Soshnikov from comment #14)
> > And this is simply wrong.
> 
> I'm sorry, but I have to disagree.

No, no, you sorry me (if my short answer seemed a bit rough) -- I just really didn't get why native functions don't obey the same laws in that case.

And regarding Object.prototype.hasOwnProperty, in my algorithm steps above (https://bugzilla.mozilla.org/show_bug.cgi?id=701364#c12) it seems all correct and |this| value in non-strict mode should arrive into the function set to the global object. Or do I miss something?

Dmitry.

Comment 18

[Jeff Walden [:Waldo]]

A built-in function does not have "function code", and 10.4.3 does not apply.  When [[Call]] for a function defined in a Program is called, that invokes 10.4.3.  But for a built-in function, the steps of its algorithm are executed instead.  A host object that implements [[Call]] does not execute the steps in 10.4.3, either.

Comment 19

[Dmitry Soshnikov]

(In reply to Jeff Walden (remove +bmo to email) from comment #18)
> A built-in function does not have "function code", and 10.4.3 does not
> apply.  When [[Call]] for a function defined in a Program is called, that
> invokes 10.4.3.  But for a built-in function, the steps of its algorithm are
> executed instead.  A host object that implements [[Call]] does not execute
> the steps in 10.4.3, either.

You want to say that when in 11.2.3 (Function calls) it's not the [[Call]] from 13.2.1, but the direct [[Call]] of native built-in functions? If so -- shame on me (seems I tired today) and my apologies; take my words back.

Dmitry.

Comment 20

[Dmitry Soshnikov]

(In reply to Dmitry A. Soshnikov from comment #19)
> (In reply to Jeff Walden (remove +bmo to email) from comment #18)
> > A built-in function does not have "function code", and 10.4.3 does not
> > apply.  When [[Call]] for a function defined in a Program is called, that
> > invokes 10.4.3.  But for a built-in function, the steps of its algorithm are
> > executed instead.  A host object that implements [[Call]] does not execute
> > the steps in 10.4.3, either.
> 
> You want to say that when in 11.2.3 (Function calls) it's not the [[Call]]
> from 13.2.1, but the direct [[Call]] of native built-in functions? If so --
> shame on me (seems I tired today) and my apologies; take my words back.
> 

Wait the second.. But how then SpiderMonkey 1.8.5 (own build, Windows) does work "correctly" and doesn't throw?

Dmitry.

Comment 21

[Jeff Walden [:Waldo]]

SM 1.8.5 predates implementation of the ES5 behavior, implementing only the ES3 behavior.  Current SM trunk implements the same thing as Firefox 8, so the next SM release will be ES5-compatible in this regard.

Comment 22

[Dmitry Soshnikov]

(In reply to Jeff Walden (remove +bmo to email) from comment #21)
> SM 1.8.5 predates implementation of the ES5 behavior, implementing only the
> ES3 behavior.  Current SM trunk implements the same thing as Firefox 8, so
> the next SM release will be ES5-compatible in this regard.

Thanks, I see.

So, the generic [[Call]] of built-ins isn't specify and we go directly to the algorithm providing context state (inc. this-value) directly from 11.2.3.

Dmitry.

Comment 23

[Thomas Fischer]

Hi,
One more question regarding 15.3.4.3 & 4 (apply & call)

The spec note "The thisArg value is passed without modification as the this value." doesn't mention the strict mode at all. But FF8 stick to this rule ONLY in strict mode:

console.log(
  (function(){return "laxly:" + this}).call(),
  (function(){"use strict";return "strict:" + this}).call()
)

Is this also not a bug but a feature?

Comment 24

[Masatoshi Kimura [:emk]]

(In reply to Thomas Fischer from comment #23)
> Hi,
> One more question regarding 15.3.4.3 & 4 (apply & call)
> 
> The spec note "The thisArg value is passed without modification as the this
> value." doesn't mention the strict mode at all. But FF8 stick to this rule
> ONLY in strict mode:
> 
> console.log(
>   (function(){return "laxly:" + this}).call(),
>   (function(){"use strict";return "strict:" + this}).call()
> )
> 
> Is this also not a bug but a feature?
Those functions are user-defined and have "function code". So thisArg will turn into global object in non-strict mode by 13.2.1 Step 1 -> 10.4.3. Step 2.