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

tty: avoid oob warning in TTYWrap::GetWindowSize() #11454

Closed
wants to merge 1 commit into from

Conversation

reklatsmasters
Copy link
Contributor

If we run node with flag --trace_array_abuse, we will see many warnings like

[OOB array elements write (array length = 1, element accessed = 1) in new ~WriteStream+877 at tty.js:59]

These logs generated by CheckArrayAbuse. If we set env NODE_DEBUG, we get these messages for tty module. Other warnings generated from process.binding and StackPush

Step to reproduce:
NODE_DEBUG=module node --trace_array_abuse -e ''
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

tty

@nodejs-github-bot nodejs-github-bot added the tty Issues and PRs related to the tty subsystem. label Feb 18, 2017
@Fishrock123
Copy link
Contributor

Fishrock123 commented Feb 21, 2017

@reklatsmasters could you make sure the commit message first line is no more than 50 characters long? :D (looks like it might be over?)

@reklatsmasters reklatsmasters changed the title tty: avoid out-of-bounds warning in TTYWrap::GetWindowSize() tty: avoid oob warning in TTYWrap::GetWindowSize() Feb 21, 2017
@reklatsmasters
Copy link
Contributor Author

@Fishrock123 Ok, replaced out-of-bounds with oob. I found the same warning in process.binding. Module process used uninitialized (with specific length) undocumented property moduleLoadList. I've not found a way to fix this warning.

@addaleax
Copy link
Member

Landed in 3f02b47 (with the commit message title set to the changed PR title here). Thanks for the PR!

@addaleax addaleax closed this Feb 22, 2017
addaleax pushed a commit that referenced this pull request Feb 22, 2017
PR-URL: #11454
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Feb 22, 2017
PR-URL: #11454
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
jasnell pushed a commit that referenced this pull request Mar 7, 2017
PR-URL: #11454
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
PR-URL: #11454
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
PR-URL: #11454
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
PR-URL: #11454
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tty Issues and PRs related to the tty subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants