Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto,tls: fix mutability of return values #10795

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 14, 2017

If you alter the array returned by tls.getCiphers(),
crypto.getCiphers(), crypto.getHashes(), or crypto.getCurves(), it
will alter subsequent return values from those functions.

'use strict';

const crypto = require('crypto');

var hashes = crypto.getHashes();

hashes.splice(0, hashes.length);

hashes.push('some-arbitrary-value');

console.log(crypto.getHashes()); // "['some-arbitrary-value']"

This is surprising. Change functions to return copy of array instead.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto tls test util

If you alter the array returned by `tls.getCiphers()`,
`crypto.getCiphers()`, `crypto.getHashes()`, or `crypto.getCurves()`, it
will alter subsequent return values from those functions.

```js
'use strict';

const crypto = require('crypto');

var hashes = crypto.getHashes();

hashes.splice(0, hashes.length);

hashes.push('some-arbitrary-value');

console.log(crypto.getHashes()); // "['some-arbitrary-value']"
```

This is surprising. Change functions to return copy of array instead.
@Trott Trott added crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem. util Issues and PRs related to the built-in util module. labels Jan 14, 2017
@nodejs-github-bot nodejs-github-bot added tls Issues and PRs related to the tls subsystem. util Issues and PRs related to the built-in util module. lts-watch-v6.x labels Jan 14, 2017
});
exports.getCiphers = internalUtil.cachedResult(
() => internalUtil.filterDuplicateStrings(binding.getSSLCiphers(), true)
);
Copy link
Member Author

@Trott Trott Jan 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reformatting is being done so it matches the formatting in lib/crypto.js so all four functions being talked about here are the same. I can take it out if it's seen as extraneous churn for this change. I'm +0 on including it. /cc @sam-github who flagged this in another PR

@Trott
Copy link
Member Author

Trott commented Jan 14, 2017

Benchmark for this change:

                                         improvement significant      p.value
 crypto/get-ciphers.js v="crypto" n=1       -16.62 %         *** 7.834831e-07
 crypto/get-ciphers.js v="crypto" n=5000    -99.84 %         *** 2.037318e-40
 crypto/get-ciphers.js v="tls" n=1          -10.02 %          ** 5.403611e-03
 crypto/get-ciphers.js v="tls" n=5000       -99.80 %         *** 1.306200e-43

For comparison, here's the benchmark if we remove the caching function entirely instead:

                                         improvement significant      p.value
 crypto/get-ciphers.js v="crypto" n=1       -81.34 %         *** 2.383610e-28
 crypto/get-ciphers.js v="crypto" n=5000    -99.99 %         *** 1.417281e-30
 crypto/get-ciphers.js v="tls" n=1          -76.18 %         *** 1.082342e-30
 crypto/get-ciphers.js v="tls" n=5000       -99.98 %         *** 1.730237e-37

The benchmarks for n=5000 seem like a low priority/concern. Conversation in the original issue where the benchmark/caching was introduced suggests that it was not based on a real-world use case and there was at least one person asking if it was really worth doing. #7225

I'm surprised that there is a perf boost with the caching function when n=1. Not sure if it's a faulty benchmark or some V8 optimization I don't understand or what is going on there. I welcome explanation from any better-informed people.

@Trott
Copy link
Member Author

Trott commented Jan 14, 2017

@sam-github
Copy link
Contributor

remove the caching function entirely instead of keeping it but using .slice() to have it return a copy of the array

I think the using slice is included in the benchmark with caching, and the removing the caching function entirely variant does not use slice?

In other words, its faster to cache and slice, then it is to just return the raw value (no caching or slicing)?

In which case, +1 from me for cache+slice. It makes the caching absolutely undetectable by the user, and is still faster than if no caching is done at all.

@Trott
Copy link
Member Author

Trott commented Jan 14, 2017

In other words, its faster to cache and slice, then it is to just return the raw value (no caching or slicing)?

That's right.

( I don't understand why that is the case for n=1, though. Why should caching the result of an action improve performance for an action you only take once?)

@Trott
Copy link
Member Author

Trott commented Jan 17, 2017

Landed in 5695067

Trott added a commit to Trott/io.js that referenced this pull request Jan 17, 2017
If you alter the array returned by `tls.getCiphers()`,
`crypto.getCiphers()`, `crypto.getHashes()`, or `crypto.getCurves()`, it
will alter subsequent return values from those functions.

```js
'use strict';

const crypto = require('crypto');

var hashes = crypto.getHashes();

hashes.splice(0, hashes.length);

hashes.push('some-arbitrary-value');

console.log(crypto.getHashes()); // "['some-arbitrary-value']"
```

This is surprising. Change functions to return copy of array instead.

PR-URL: nodejs#10795
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@Trott Trott closed this Jan 17, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
If you alter the array returned by `tls.getCiphers()`,
`crypto.getCiphers()`, `crypto.getHashes()`, or `crypto.getCurves()`, it
will alter subsequent return values from those functions.

```js
'use strict';

const crypto = require('crypto');

var hashes = crypto.getHashes();

hashes.splice(0, hashes.length);

hashes.push('some-arbitrary-value');

console.log(crypto.getHashes()); // "['some-arbitrary-value']"
```

This is surprising. Change functions to return copy of array instead.

PR-URL: nodejs#10795
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017
If you alter the array returned by `tls.getCiphers()`,
`crypto.getCiphers()`, `crypto.getHashes()`, or `crypto.getCurves()`, it
will alter subsequent return values from those functions.

```js
'use strict';

const crypto = require('crypto');

var hashes = crypto.getHashes();

hashes.splice(0, hashes.length);

hashes.push('some-arbitrary-value');

console.log(crypto.getHashes()); // "['some-arbitrary-value']"
```

This is surprising. Change functions to return copy of array instead.

PR-URL: nodejs#10795
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
If you alter the array returned by `tls.getCiphers()`,
`crypto.getCiphers()`, `crypto.getHashes()`, or `crypto.getCurves()`, it
will alter subsequent return values from those functions.

```js
'use strict';

const crypto = require('crypto');

var hashes = crypto.getHashes();

hashes.splice(0, hashes.length);

hashes.push('some-arbitrary-value');

console.log(crypto.getHashes()); // "['some-arbitrary-value']"
```

This is surprising. Change functions to return copy of array instead.

PR-URL: nodejs#10795
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
If you alter the array returned by `tls.getCiphers()`,
`crypto.getCiphers()`, `crypto.getHashes()`, or `crypto.getCurves()`, it
will alter subsequent return values from those functions.

```js
'use strict';

const crypto = require('crypto');

var hashes = crypto.getHashes();

hashes.splice(0, hashes.length);

hashes.push('some-arbitrary-value');

console.log(crypto.getHashes()); // "['some-arbitrary-value']"
```

This is surprising. Change functions to return copy of array instead.

PR-URL: nodejs#10795
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
If you alter the array returned by `tls.getCiphers()`,
`crypto.getCiphers()`, `crypto.getHashes()`, or `crypto.getCurves()`, it
will alter subsequent return values from those functions.

```js
'use strict';

const crypto = require('crypto');

var hashes = crypto.getHashes();

hashes.splice(0, hashes.length);

hashes.push('some-arbitrary-value');

console.log(crypto.getHashes()); // "['some-arbitrary-value']"
```

This is surprising. Change functions to return copy of array instead.

PR-URL: #10795
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
If you alter the array returned by `tls.getCiphers()`,
`crypto.getCiphers()`, `crypto.getHashes()`, or `crypto.getCurves()`, it
will alter subsequent return values from those functions.

```js
'use strict';

const crypto = require('crypto');

var hashes = crypto.getHashes();

hashes.splice(0, hashes.length);

hashes.push('some-arbitrary-value');

console.log(crypto.getHashes()); // "['some-arbitrary-value']"
```

This is surprising. Change functions to return copy of array instead.

PR-URL: #10795
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
@Trott Trott deleted the immutable branch January 13, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants