Skip to content

Commit

Permalink
events: don't inherit from Object.prototype
Browse files Browse the repository at this point in the history
This commit safely allows event names that are named the same as
properties that are ordinarily inherited from Object.prototype such
as __proto__.

Fixes: #728
PR-URL: #6092
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
mscdex committed Apr 19, 2016
1 parent dba245f commit e38bade
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 21 deletions.
20 changes: 13 additions & 7 deletions lib/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@

var domain;

// This constructor is used to store event handlers. Instantiating this is
// faster than explicitly calling `Object.create(null)` to get a "clean" empty
// object (tested with v8 v4.9).
function EventHandlers() {}
EventHandlers.prototype = Object.create(null);

function EventEmitter() {
EventEmitter.init.call(this);
}
Expand Down Expand Up @@ -44,7 +50,7 @@ EventEmitter.init = function() {
}

if (!this._events || this._events === Object.getPrototypeOf(this)._events) {
this._events = {};
this._events = new EventHandlers();
this._eventsCount = 0;
}

Expand Down Expand Up @@ -211,7 +217,7 @@ EventEmitter.prototype.addListener = function addListener(type, listener) {

events = this._events;
if (!events) {
events = this._events = {};
events = this._events = new EventHandlers();
this._eventsCount = 0;
} else {
// To avoid recursion in the case that type === "newListener"! Before
Expand Down Expand Up @@ -296,7 +302,7 @@ EventEmitter.prototype.removeListener =

if (list === listener || (list.listener && list.listener === listener)) {
if (--this._eventsCount === 0)
this._events = {};
this._events = new EventHandlers();
else {
delete events[type];
if (events.removeListener)
Expand All @@ -319,7 +325,7 @@ EventEmitter.prototype.removeListener =
if (list.length === 1) {
list[0] = undefined;
if (--this._eventsCount === 0) {
this._events = {};
this._events = new EventHandlers();
return this;
} else {
delete events[type];
Expand All @@ -346,11 +352,11 @@ EventEmitter.prototype.removeAllListeners =
// not listening for removeListener, no need to emit
if (!events.removeListener) {
if (arguments.length === 0) {
this._events = {};
this._events = new EventHandlers();
this._eventsCount = 0;
} else if (events[type]) {
if (--this._eventsCount === 0)
this._events = {};
this._events = new EventHandlers();
else
delete events[type];
}
Expand All @@ -366,7 +372,7 @@ EventEmitter.prototype.removeAllListeners =
this.removeAllListeners(key);
}
this.removeAllListeners('removeListener');
this._events = {};
this._events = new EventHandlers();
this._eventsCount = 0;
return this;
}
Expand Down
4 changes: 3 additions & 1 deletion src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3228,7 +3228,9 @@ void SetupProcessObject(Environment* env,
env->SetMethod(process, "_setupDomainUse", SetupDomainUse);

// pre-set _events object for faster emit checks
process->Set(env->events_string(), Object::New(env->isolate()));
Local<Object> events_obj = Object::New(env->isolate());
events_obj->SetPrototype(env->context(), Null(env->isolate()));

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 19, 2016

Member

@mscdex ... fyi... this is causing a compiler warning when building

../src/node.cc:3232:3: warning: ignoring return value of function declared with
      warn_unused_result attribute [-Wunused-result]
  events_obj->SetPrototype(env->context(), Null(env->isolate()));
  ^~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
process->Set(env->events_string(), events_obj);
}


Expand Down
12 changes: 0 additions & 12 deletions test/known_issues/test-events-known-properties.js

This file was deleted.

1 change: 0 additions & 1 deletion test/parallel/test-event-emitter-listeners-side-effects.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ assert(fl.length === 1);
assert(fl[0] === assert.fail);

e.listeners('bar');
assert(!e._events.hasOwnProperty('bar'));

e.on('foo', assert.ok);
fl = e.listeners('foo');
Expand Down
36 changes: 36 additions & 0 deletions test/parallel/test-event-emitter-special-event-names.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
'use strict';

const common = require('../common');
const EventEmitter = require('events');
const assert = require('assert');

const ee = new EventEmitter();
const handler = () => {};

assert.strictEqual(ee._events.hasOwnProperty, undefined);
assert.strictEqual(ee._events.toString, undefined);

ee.on('__proto__', handler);
ee.on('__defineGetter__', handler);
ee.on('toString', handler);

assert.deepStrictEqual(ee.eventNames(), [
'__proto__',
'__defineGetter__',
'toString'
]);

assert.deepStrictEqual(ee.listeners('__proto__'), [handler]);
assert.deepStrictEqual(ee.listeners('__defineGetter__'), [handler]);
assert.deepStrictEqual(ee.listeners('toString'), [handler]);

ee.on('__proto__', common.mustCall(function(val) {
assert.strictEqual(val, 1);
}));
ee.emit('__proto__', 1);

process.on('__proto__', common.mustCall(function(val) {
assert.strictEqual(val, 1);
}));
process.emit('__proto__', 1);

0 comments on commit e38bade

Please sign in to comment.