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

zlib: be strict about what strategies are accepted #10934

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@Trott
Member

Trott commented Jan 21, 2017

Currently, strategy constants are integers but Node.js will accept
string versions of those integers. Users should be using the provided
zlib constants and not hardcoding numbers, strings, or anything else. As
such, Node.js should be strict about accepting only exactly those values
that are in the provided zlib constants.

Fixes: #10932

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)

zlib

zlib: be strict about what strategies are accepted
Currently, strategy constants are integers but Node.js will accept
string versions of those integers. Users should be using the provided
zlib constants and not hardcoding numbers, strings, or anything else. As
such, Node.js should be strict about accepting only exactly those values
that are in the provided zlib constants.

Fixes: #10932
@targos

targos approved these changes Jan 21, 2017

throw new Error('Invalid strategy: ' + opts.strategy);
}
}
if (opts.strategy && !(strategies.includes(opts.strategy)))

This comment has been minimized.

@lpinca

lpinca Jan 21, 2017

Member

Nit: why !(strategies.includes(opts.strategy)) instead of just !strategies.includes(opts.strategy)?

@lpinca

lpinca Jan 21, 2017

Member

Nit: why !(strategies.includes(opts.strategy)) instead of just !strategies.includes(opts.strategy)?

This comment has been minimized.

@Trott

Trott Jan 23, 2017

Member

Happy to remove it if it's bothersome, but the reason I (sometimes) use the parentheses like that are:

  • clarity of intention
  • helps out people who may not know operator precedence
  • force of habit from !(foo instanceof Buffer) being the correct way to do things because !foo instanceof Buffer means checking that !foo is an instance of Buffer which is not at all the same as foo not an instances of Buffer....
@Trott

Trott Jan 23, 2017

Member

Happy to remove it if it's bothersome, but the reason I (sometimes) use the parentheses like that are:

  • clarity of intention
  • helps out people who may not know operator precedence
  • force of habit from !(foo instanceof Buffer) being the correct way to do things because !foo instanceof Buffer means checking that !foo is an instance of Buffer which is not at all the same as foo not an instances of Buffer....

This comment has been minimized.

@lpinca

lpinca Jan 23, 2017

Member

No problem, keep it the way it is.

@lpinca

lpinca Jan 23, 2017

Member

No problem, keep it the way it is.

This comment has been minimized.

@jasnell

jasnell Jan 23, 2017

Member

A range check may be faster than includes ...e.g.

if (opts.strategy >= constants.Z_DEFAULT_STRATEGY &&
   opt.strategy <= constants.Z_FIXED) { ... }

A bit more work to maintain if these values ever change but it avoids the linear scan of the array that includes() requires.

@jasnell

jasnell Jan 23, 2017

Member

A range check may be faster than includes ...e.g.

if (opts.strategy >= constants.Z_DEFAULT_STRATEGY &&
   opt.strategy <= constants.Z_FIXED) { ... }

A bit more work to maintain if these values ever change but it avoids the linear scan of the array that includes() requires.

This comment has been minimized.

@Trott

Trott Jan 23, 2017

Member

Given the overhead of actually compressing and/or decompressing, I suspect such a change for performance purposes in the constructor would be something that would be impossible to measure in any real-world scenario. Counter-examples more than welcome. Barring that, I would prefer the code be clear and not be dependent on unspoken assumptions about constants defined externally.

@Trott

Trott Jan 23, 2017

Member

Given the overhead of actually compressing and/or decompressing, I suspect such a change for performance purposes in the constructor would be something that would be impossible to measure in any real-world scenario. Counter-examples more than welcome. Barring that, I would prefer the code be clear and not be dependent on unspoken assumptions about constants defined externally.

This comment has been minimized.

@jasnell

jasnell Jan 24, 2017

Member

works for me

@jasnell

jasnell Jan 24, 2017

Member

works for me

@lpinca

lpinca approved these changes Jan 21, 2017

@Trott Trott referenced this pull request Jan 23, 2017

Closed

test,zlib: check string strategies #10933

3 of 3 tasks complete
@Trott

This comment has been minimized.

Show comment
Hide comment

Trott added a commit to Trott/io.js that referenced this pull request Jan 25, 2017

zlib: be strict about what strategies are accepted
Currently, strategy constants are integers but Node.js will accept
string versions of those integers. Users should be using the provided
zlib constants and not hardcoding numbers, strings, or anything else. As
such, Node.js should be strict about accepting only exactly those values
that are in the provided zlib constants.

PR-URL: nodejs#10934
Fixes: nodejs#10932
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Jan 25, 2017

Member

Landed in dd928b0

Member

Trott commented Jan 25, 2017

Landed in dd928b0

@Trott Trott closed this Jan 25, 2017

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment