From 9f8f0c39307fd22e8a8912d1946c946f598eb98e Mon Sep 17 00:00:00 2001 From: Andrea Giammarchi Date: Thu, 10 Mar 2016 23:39:27 +0000 Subject: [PATCH 1/4] avoiding any sort of possible conflict with the Object.prototype --- lib/querystring.js | 87 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 61 insertions(+), 26 deletions(-) diff --git a/lib/querystring.js b/lib/querystring.js index b56ad77012d035..2b4e3ce6449dea 100644 --- a/lib/querystring.js +++ b/lib/querystring.js @@ -4,6 +4,7 @@ const QueryString = exports; const Buffer = require('buffer').Buffer; +const defineProperty = Object.defineProperty; // a safe fast alternative to decodeURIComponent @@ -267,20 +268,37 @@ QueryString.parse = QueryString.decode = function(qs, sep, eq, options) { key = decodeStr(key, decode); if (valEncoded) value = decodeStr(value, decode); - // Use a key array lookup instead of using hasOwnProperty(), which is - // slower - if (keys.indexOf(key) === -1) { - obj[key] = value; - keys[keys.length] = key; + // most common case is to define unique keys per query string + // using the `in` operator we speed up the assignment of unique values + // instead of performing an indexOf each time. + // On top of that, we are sure the property is not conflicting + // with those in Object.prototype so it's also safer. + // However, if the object had already a key, even as own property + // we need to check the indexOf instantly after. + if (key in obj) { + // Use a key array lookup instead of using hasOwnProperty(), + // which is slower + if (keys.indexOf(key) === -1) { + keys[keys.length] = key; + defineProperty(obj, key, { + configurable: true, + enumerable: true, + writable: true, + value: value + }); + } else { + const curValue = obj[key]; + // `instanceof Array` is used instead of Array.isArray() because it + // is ~15-20% faster with v8 4.7 and is safe to use because we are + // using it with values being created within this function + if (curValue instanceof Array) + curValue[curValue.length] = value; + else + obj[key] = [curValue, value]; + } } else { - const curValue = obj[key]; - // `instanceof Array` is used instead of Array.isArray() because it - // is ~15-20% faster with v8 4.7 and is safe to use because we are - // using it with values being created within this function - if (curValue instanceof Array) - curValue[curValue.length] = value; - else - obj[key] = [curValue, value]; + keys[keys.length] = key; + obj[key] = value; } if (--pairs === 0) break; @@ -370,20 +388,37 @@ QueryString.parse = QueryString.decode = function(qs, sep, eq, options) { key = decodeStr(key, decode); if (valEncoded) value = decodeStr(value, decode); - // Use a key array lookup instead of using hasOwnProperty(), which is - // slower - if (keys.indexOf(key) === -1) { - obj[key] = value; - keys[keys.length] = key; + // most common case is to define unique keys per query string + // using the `in` operator we speed up the assignment of unique values + // instead of performing an indexOf each time. + // On top of that, we are sure the property is not conflicting + // with those in Object.prototype so it's also safer. + // However, if the object had already a key, even as own property + // we need to check the indexOf instantly after. + if (key in obj) { + // Use a key array lookup instead of using hasOwnProperty(), + // which is slower + if (keys.indexOf(key) === -1) { + keys[keys.length] = key; + defineProperty(obj, key, { + configurable: true, + enumerable: true, + writable: true, + value: value + }); + } else { + const curValue = obj[key]; + // `instanceof Array` is used instead of Array.isArray() because it + // is ~15-20% faster with v8 4.7 and is safe to use because we are + // using it with values being created within this function + if (curValue instanceof Array) + curValue[curValue.length] = value; + else + obj[key] = [curValue, value]; + } } else { - const curValue = obj[key]; - // `instanceof Array` is used instead of Array.isArray() because it - // is ~15-20% faster with v8 4.7 and is safe to use because we are - // using it with values being created within this function - if (curValue instanceof Array) - curValue[curValue.length] = value; - else - obj[key] = [curValue, value]; + keys[keys.length] = key; + obj[key] = value; } } From 9eab7717757981305c7d3236aaaea0b88b38b0bb Mon Sep 17 00:00:00 2001 From: Andrea Giammarchi Date: Fri, 11 Mar 2016 00:35:07 +0000 Subject: [PATCH 2/4] made __proto__ test working via JSON.parse --- test/parallel/test-querystring.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/parallel/test-querystring.js b/test/parallel/test-querystring.js index c8e9cc7050af5b..397d165fb9efac 100644 --- a/test/parallel/test-querystring.js +++ b/test/parallel/test-querystring.js @@ -9,6 +9,9 @@ var qs = require('querystring'); // {{{ // [ wonkyQS, canonicalQS, obj ] var qsTestCases = [ + ['__proto__=1', + '__proto__=1', + JSON.parse('{"__proto__":"1"}')], ['foo=918854443121279438895193', 'foo=918854443121279438895193', {'foo': '918854443121279438895193'}], From b12181677b2a873e0e24cb62687a04af5258e840 Mon Sep 17 00:00:00 2001 From: Andrea Giammarchi Date: Thu, 10 Mar 2016 23:39:27 +0000 Subject: [PATCH 3/4] querystring: avoid conflicts with Object.prototype and __proto__ In order to solve #5642 , keeping an eye and actually most likely improving performance for #728, this PR is meant to show a better way to deal with inheritance. As most common CS related problem say, "hashes" are faster than O(N*?) searches in an array and this is the reason I've proposed a solution based on `in` operator first. --- lib/querystring.js | 87 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 61 insertions(+), 26 deletions(-) diff --git a/lib/querystring.js b/lib/querystring.js index b56ad77012d035..2b4e3ce6449dea 100644 --- a/lib/querystring.js +++ b/lib/querystring.js @@ -4,6 +4,7 @@ const QueryString = exports; const Buffer = require('buffer').Buffer; +const defineProperty = Object.defineProperty; // a safe fast alternative to decodeURIComponent @@ -267,20 +268,37 @@ QueryString.parse = QueryString.decode = function(qs, sep, eq, options) { key = decodeStr(key, decode); if (valEncoded) value = decodeStr(value, decode); - // Use a key array lookup instead of using hasOwnProperty(), which is - // slower - if (keys.indexOf(key) === -1) { - obj[key] = value; - keys[keys.length] = key; + // most common case is to define unique keys per query string + // using the `in` operator we speed up the assignment of unique values + // instead of performing an indexOf each time. + // On top of that, we are sure the property is not conflicting + // with those in Object.prototype so it's also safer. + // However, if the object had already a key, even as own property + // we need to check the indexOf instantly after. + if (key in obj) { + // Use a key array lookup instead of using hasOwnProperty(), + // which is slower + if (keys.indexOf(key) === -1) { + keys[keys.length] = key; + defineProperty(obj, key, { + configurable: true, + enumerable: true, + writable: true, + value: value + }); + } else { + const curValue = obj[key]; + // `instanceof Array` is used instead of Array.isArray() because it + // is ~15-20% faster with v8 4.7 and is safe to use because we are + // using it with values being created within this function + if (curValue instanceof Array) + curValue[curValue.length] = value; + else + obj[key] = [curValue, value]; + } } else { - const curValue = obj[key]; - // `instanceof Array` is used instead of Array.isArray() because it - // is ~15-20% faster with v8 4.7 and is safe to use because we are - // using it with values being created within this function - if (curValue instanceof Array) - curValue[curValue.length] = value; - else - obj[key] = [curValue, value]; + keys[keys.length] = key; + obj[key] = value; } if (--pairs === 0) break; @@ -370,20 +388,37 @@ QueryString.parse = QueryString.decode = function(qs, sep, eq, options) { key = decodeStr(key, decode); if (valEncoded) value = decodeStr(value, decode); - // Use a key array lookup instead of using hasOwnProperty(), which is - // slower - if (keys.indexOf(key) === -1) { - obj[key] = value; - keys[keys.length] = key; + // most common case is to define unique keys per query string + // using the `in` operator we speed up the assignment of unique values + // instead of performing an indexOf each time. + // On top of that, we are sure the property is not conflicting + // with those in Object.prototype so it's also safer. + // However, if the object had already a key, even as own property + // we need to check the indexOf instantly after. + if (key in obj) { + // Use a key array lookup instead of using hasOwnProperty(), + // which is slower + if (keys.indexOf(key) === -1) { + keys[keys.length] = key; + defineProperty(obj, key, { + configurable: true, + enumerable: true, + writable: true, + value: value + }); + } else { + const curValue = obj[key]; + // `instanceof Array` is used instead of Array.isArray() because it + // is ~15-20% faster with v8 4.7 and is safe to use because we are + // using it with values being created within this function + if (curValue instanceof Array) + curValue[curValue.length] = value; + else + obj[key] = [curValue, value]; + } } else { - const curValue = obj[key]; - // `instanceof Array` is used instead of Array.isArray() because it - // is ~15-20% faster with v8 4.7 and is safe to use because we are - // using it with values being created within this function - if (curValue instanceof Array) - curValue[curValue.length] = value; - else - obj[key] = [curValue, value]; + keys[keys.length] = key; + obj[key] = value; } } From baca0faa8611a84ac08274b54e208df2185c69a0 Mon Sep 17 00:00:00 2001 From: Andrea Giammarchi Date: Fri, 11 Mar 2016 00:35:07 +0000 Subject: [PATCH 4/4] made __proto__ test working via JSON.parse --- test/parallel/test-querystring.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/parallel/test-querystring.js b/test/parallel/test-querystring.js index c8e9cc7050af5b..397d165fb9efac 100644 --- a/test/parallel/test-querystring.js +++ b/test/parallel/test-querystring.js @@ -9,6 +9,9 @@ var qs = require('querystring'); // {{{ // [ wonkyQS, canonicalQS, obj ] var qsTestCases = [ + ['__proto__=1', + '__proto__=1', + JSON.parse('{"__proto__":"1"}')], ['foo=918854443121279438895193', 'foo=918854443121279438895193', {'foo': '918854443121279438895193'}],