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.

Sign up for GitHub

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

Jump to bottom

double execution with THROW_AND_RETURN_IF_OOB #41935

Closed
powimod opened this issue Feb 11, 2022 · 1 comment · Fixed by #41945
Closed

double execution with THROW_AND_RETURN_IF_OOB #41935

powimod opened this issue Feb 11, 2022 · 1 comment · Fixed by #41945
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++.

Comments

@powimod
Copy link

powimod commented Feb 11, 2022

Version

16.13.2

Platform

Microsoft Windows NT 10.0.19042.0 x64

Subsystem

No response

What steps will reproduce the bug?

In file node_buffer.cc, macro THROW_AND_RETURN_IF_OOB is called with a function as argument :

  THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[0], 0, &start));

This macro is replaced with this code :

  do {   \
    if ((ParseArrayIndex(env, args[0], 0, &start)).IsNothing()) return;  \
    if (!(ParseArrayIndex(env, args[0], 0, &start)).FromJust()) \
      return node::THROW_ERR_OUT_OF_RANGE(env, "Index out of range");       \
  } while (0)

The ParseArrayIndex function will be called twice which will generate performance losses.

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

No response

What do you see instead?

Each macro call of THROW_AND_RETURN_IF_OOB macro should be checked.

Additional information

No response
@Mesteery Mesteery added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Feb 11, 2022
RaisinTen added a commit to RaisinTen/node that referenced this issue Feb 12, 2022
@RaisinTen
src,buffer: evaluate THROW_AND_RETURN_IF_OOB() expression only once
b357ed1 
There's no need to evaluate the expression passed into the macro more
than once.

Fixes: nodejs#41935
Signed-off-by: Darshan Sen <[email protected]>
@RaisinTen RaisinTen mentioned this issue Feb 12, 2022
Merged
@RaisinTen
Copy link
Contributor

PR: #41945

aduh95 pushed a commit that referenced this issue Feb 17, 2022
@RaisinTen
src,buffer: evaluate THROW_AND_RETURN_IF_OOB() expression only once
5edad38 
There's no need to evaluate the expression passed into the macro more
than once.

Fixes: #41935
Signed-off-by: Darshan Sen <[email protected]>

PR-URL: #41945
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
bengl pushed a commit to bengl/node that referenced this issue Feb 21, 2022
@RaisinTen @bengl
src,buffer: evaluate THROW_AND_RETURN_IF_OOB() expression only once
c76fd56 
There's no need to evaluate the expression passed into the macro more
than once.

Fixes: nodejs#41935
Signed-off-by: Darshan Sen <[email protected]>

PR-URL: nodejs#41945
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
bengl pushed a commit to bengl/node that referenced this issue Feb 21, 2022
@RaisinTen @bengl
src,buffer: evaluate THROW_AND_RETURN_IF_OOB() expression only once
7876549 
There's no need to evaluate the expression passed into the macro more
than once.

Fixes: nodejs#41935
Signed-off-by: Darshan Sen <[email protected]>

PR-URL: nodejs#41945
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
bengl pushed a commit that referenced this issue Feb 21, 2022
@RaisinTen @bengl
src,buffer: evaluate THROW_AND_RETURN_IF_OOB() expression only once
86c8230 
There's no need to evaluate the expression passed into the macro more
than once.

Fixes: #41935
Signed-off-by: Darshan Sen <[email protected]>

PR-URL: #41945
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
bengl pushed a commit that referenced this issue Feb 21, 2022
@RaisinTen @bengl
src,buffer: evaluate THROW_AND_RETURN_IF_OOB() expression only once
047d4de 
There's no need to evaluate the expression passed into the macro more
than once.

Fixes: #41935
Signed-off-by: Darshan Sen <[email protected]>

PR-URL: #41945
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
bengl pushed a commit that referenced this issue Feb 22, 2022
@RaisinTen @bengl
src,buffer: evaluate THROW_AND_RETURN_IF_OOB() expression only once
43c12f4 
There's no need to evaluate the expression passed into the macro more
than once.

Fixes: #41935
Signed-off-by: Darshan Sen <[email protected]>

PR-URL: #41945
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
danielleadams pushed a commit to danielleadams/node that referenced this issue Apr 21, 2022
@RaisinTen @danielleadams
src,buffer: evaluate THROW_AND_RETURN_IF_OOB() expression only once
a2b0e1f 
There's no need to evaluate the expression passed into the macro more
than once.

Fixes: nodejs#41935
Signed-off-by: Darshan Sen <[email protected]>

PR-URL: nodejs#41945
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
@RaisinTen @danielleadams
src,buffer: evaluate THROW_AND_RETURN_IF_OOB() expression only once
fdea60e 
There's no need to evaluate the expression passed into the macro more
than once.

Fixes: #41935
Signed-off-by: Darshan Sen <[email protected]>

PR-URL: #41945
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

src,buffer: evaluate THROW_AND_RETURN_IF_OOB() expression only once RaisinTen/node
3 participants
@powimod @RaisinTen @Mesteery