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

src: fix compiler warnings in node_buffer.cc #25665

Closed
wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Jan 23, 2019

Currently the following compiler warnings are generated on Linux:

../src/node_buffer.cc:
In function 'void node::Buffer::{anonymous}::StringSlice(
    const v8::FunctionCallbackInfo<v8::Value>&)
      [with node::encoding encoding = (node::encoding)1]':
../src/node_buffer.cc:54:20: warning:
'start' may be used uninitialized in this function
[-Wmaybe-uninitialized]
   if (end < start) end = start;
                    ^~~
../src/node_buffer.cc:50:10: note: 'start' was declared here
   size_t start;
          ^~~~~

This commit initializes start and end to zero to avoid these warnings.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Currently the following compiler warnings are generated on Linux:
../src/node_buffer.cc:
In function 'void node::Buffer::{anonymous}::StringSlice(
    const v8::FunctionCallbackInfo<v8::Value>&)
      [with node::encoding encoding = (node::encoding)1]':
../src/node_buffer.cc:54:20: warning:
'start' may be used uninitialized in this function
[-Wmaybe-uninitialized]
   if (end < start) end = start;
                    ^~~
../src/node_buffer.cc:50:10: note: 'start' was declared here
   size_t start;
          ^~~~~

This commit initializes start and end to zero to avoid these warnings.
@nodejs-github-bot nodejs-github-bot 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 Jan 23, 2019
@danbev
Copy link
Contributor Author

danbev commented Jan 23, 2019

@danbev
Copy link
Contributor Author

danbev commented Jan 24, 2019

Re-run of failing node-test-commit-arm-fanned :

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/20359/

Did you check what path led to these variables potentially being used while initialized? I’d be curious whether this was an actual bug in our code…

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 27, 2019
@danbev
Copy link
Contributor Author

danbev commented Jan 28, 2019

Did you check what path led to these variables potentially being used while initialized?

I can't see that these variables would ever be uninitialized, as that initialization happens in ParseArrayIndex and then the return value is guarded by the THROW_AND_RETURN_IF_OOB macro. But I'll take a closer look as I did not do a closer inspection or debugging (I was mainly trying to silence this warning similar to what is done for other compilers like clang which does not generate a warning for these statements).

From looking into this I can't find that there was a bug. The StringSlice function which uses the SLICE_START_END macro is used by asciiSlice, base64Slice, latin1Slice, hexSlice, ucsSlice, and utfSlice. These are all called from lib/buffer.js stringSlice which is only called by Buffer.prototype.toString which checks all the input arguments.

For CompareOffset is called by Buffer.prototype.compare and also checks the input arguments.

@danbev
Copy link
Contributor Author

danbev commented Feb 7, 2019

Landed in d310d8d.

@danbev danbev closed this Feb 7, 2019
@danbev danbev deleted the buffer_uninitialized_warnings branch February 7, 2019 04:40
danbev added a commit that referenced this pull request Feb 7, 2019
Currently the following compiler warnings are generated on Linux:
../src/node_buffer.cc:
In function 'void node::Buffer::{anonymous}::StringSlice(
    const v8::FunctionCallbackInfo<v8::Value>&)
      [with node::encoding encoding = (node::encoding)1]':
../src/node_buffer.cc:54:20: warning:
'start' may be used uninitialized in this function
[-Wmaybe-uninitialized]
   if (end < start) end = start;
                    ^~~
../src/node_buffer.cc:50:10: note: 'start' was declared here
   size_t start;
          ^~~~~

This commit initializes start and end to zero to avoid these warnings.

PR-URL: #25665
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 7, 2019
Currently the following compiler warnings are generated on Linux:
../src/node_buffer.cc:
In function 'void node::Buffer::{anonymous}::StringSlice(
    const v8::FunctionCallbackInfo<v8::Value>&)
      [with node::encoding encoding = (node::encoding)1]':
../src/node_buffer.cc:54:20: warning:
'start' may be used uninitialized in this function
[-Wmaybe-uninitialized]
   if (end < start) end = start;
                    ^~~
../src/node_buffer.cc:50:10: note: 'start' was declared here
   size_t start;
          ^~~~~

This commit initializes start and end to zero to avoid these warnings.

PR-URL: #25665
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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
Development

Successfully merging this pull request may close these issues.

7 participants