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

Segmentation fault when window is resized #4291

Closed
anseki opened this issue Dec 15, 2015 · 9 comments · Fixed by #5994
Closed

Segmentation fault when window is resized #4291

anseki opened this issue Dec 15, 2015 · 9 comments · Fixed by #5994
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. libuv Issues and PRs related to the libuv dependency or the uv binding.

Comments

@anseki
Copy link

anseki commented Dec 15, 2015

The process throws "Segmentation fault" if an user resize a window when a script is waiting for user input via TTY, after it accesses to process.stdout.

For example:

var
  fs = require('fs'),
  bufferSize = 1024,
  buffer = new Buffer(bufferSize),
  readSize,
  chunk;

readSize = fs.readSync(fs.openSync('/dev/tty', 'r'), buffer, 0, bufferSize);
chunk = buffer.toString('utf8', 0, readSize);

console.log('INPUT: ' + chunk);

This code above works fine without error.

foo
INPUT: foo

But, if process.stdout is accessed:

var
  fs = require('fs'),
  bufferSize = 1024,
  buffer = new Buffer(bufferSize),
  readSize,
  chunk;

process.stdout; // access to property

readSize = fs.readSync(fs.openSync('/dev/tty', 'r'), buffer, 0, bufferSize);
chunk = buffer.toString('utf8', 0, readSize);

console.log('INPUT: ' + chunk);

If a window is resized when the script is waiting for user input, it throws "Segmentation fault".

I tried this code in some versions, and it seems that this issue occurs in v3.3.0+.
v3.2.0 is OK.
v3.3.0 makes this issue occur.

Is this a problem of libuv v1.7.3 ?

@anseki
Copy link
Author

anseki commented Dec 15, 2015

This issue occurs at least in OS X 10 and Ubuntu.
I don't know a case in Windows because Command Window of Windows can't resize window, maybe.

@evanlucas
Copy link
Contributor

hm this is what I'm getting in lldb:

Process 8536 stopped
* thread #1: tid = 0x1057ca, 0x0000000100c1227a node_g`uv__fs_read(req=0x00007fff5fbfe648) + 74 at fs.c:275, queue = 'com.apple.main-thread, stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x0000000100c1227a node_g`uv__fs_read(req=0x00007fff5fbfe648) + 74 at fs.c:275
   272  #endif /* defined(_AIX) */
   273    if (req->off < 0) {
   274      if (req->nbufs == 1)
-> 275        result = read(req->file, req->bufs[0].base, req->bufs[0].len);
   276      else
   277        result = readv(req->file, (struct iovec*) req->bufs, req->nbufs);
   278    } else {

It looks like req->nbufs is 1, but req->bufs is NULL

@Fishrock123 Fishrock123 added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 15, 2015
@mscdex mscdex added the libuv Issues and PRs related to the libuv dependency or the uv binding. label Dec 15, 2015
davidvgalbraith pushed a commit to davidvgalbraith/libuv that referenced this issue Dec 20, 2015
uv__fs_buf_iter currently sets req->bufs to NULL
after it is done, but if the operation fails with EINTR
then it will be retried, at which point it expects
the bufs to not be NULL, causing a seg fault as in
nodejs/node#4291.

uv__fs_buf_iter should not set req->bufs to NULL
if the operation fails with EINTR.

Also, when it sets req->bufs to NULL, it should set
req->nbufs to 0 as well, so we don't have the messy
situation of a positive nbufs with no actual bufs.
davidvgalbraith pushed a commit to davidvgalbraith/libuv that referenced this issue Dec 20, 2015
uv__fs_buf_iter currently sets req->bufs to NULL
after it is done, but if the operation fails with EINTR
then it will be retried, at which point it expects
the bufs to not be NULL, causing a seg fault as in
nodejs/node#4291.

uv__fs_buf_iter should not set req->bufs to NULL
if the operation fails with EINTR.

Also, when it sets req->bufs to NULL, it should set
req->nbufs to 0 as well, so we don't have the messy
situation of a positive nbufs with no actual bufs.
davidvgalbraith pushed a commit to davidvgalbraith/libuv that referenced this issue Dec 23, 2015
uv__fs_buf_iter currently sets req->bufs to NULL after it is done, but
if the operation fails with EINTR then it will be retried, at which
point it expects the bufs to not be NULL, causing a seg fault as in
nodejs/node#4291.

uv__fs_buf_iter should not set req->bufs to NULL if the operation
fails with EINTR.

Also, when it sets req->bufs to NULL, it should set req->nbufs to 0 as
well, so we don't have the messy situation of a positive nbufs with no
actual bufs.

Review followups
-move the check for EINTR to before we free bufs
-pass retry_on_eintr to uv__fs_buf_iter
-add a first cut at a test
davidvgalbraith pushed a commit to davidvgalbraith/libuv that referenced this issue Dec 23, 2015
uv__fs_buf_iter currently sets req->bufs to NULL after it is done, but
if the operation fails with EINTR then it will be retried, at which
point it expects the bufs to not be NULL, causing a seg fault as in
nodejs/node#4291.

uv__fs_buf_iter should not set req->bufs to NULL if the operation
fails with EINTR.

Also, when it sets req->bufs to NULL, it should set req->nbufs to 0 as
well, so we don't have the messy situation of a positive nbufs with no
actual bufs.

Review followups
-move the check for EINTR to before we free bufs
-pass retry_on_eintr to uv__fs_buf_iter
-add a first cut at a test
davidvgalbraith pushed a commit to davidvgalbraith/libuv that referenced this issue Dec 23, 2015
uv__fs_buf_iter currently sets req->bufs to NULL after it is done, but
if the operation fails with EINTR then it will be retried, at which
point it expects the bufs to not be NULL, causing a seg fault as in
nodejs/node#4291.

uv__fs_buf_iter should not set req->bufs to NULL if the operation
fails with EINTR.

Also, when it sets req->bufs to NULL, it should set req->nbufs to 0 as
well, so we don't have the messy situation of a positive nbufs with no
actual bufs.
davidvgalbraith pushed a commit to davidvgalbraith/libuv that referenced this issue Dec 23, 2015
uv__fs_buf_iter currently sets req->bufs to NULL after it is done, but
if the operation fails with EINTR then it will be retried, at which
point it expects the bufs to not be NULL, causing a seg fault as in
nodejs/node#4291.

uv__fs_buf_iter should not set req->bufs to NULL if the operation
fails with EINTR.

Also, when it sets req->bufs to NULL, it should set req->nbufs to 0 as
well, so we don't have the messy situation of a positive nbufs with no
actual bufs.
davidvgalbraith pushed a commit to davidvgalbraith/libuv that referenced this issue Dec 24, 2015
uv__fs_buf_iter currently sets req->bufs to NULL after it is done, but
if the operation fails with EINTR then it will be retried, at which
point it expects the bufs to not be NULL, causing a seg fault as in
nodejs/node#4291.

uv__fs_buf_iter should not set req->bufs to NULL if the operation
fails with EINTR.

Also, when it sets req->bufs to NULL, it should set req->nbufs to 0 as
well, so we don't have the messy situation of a positive nbufs with no
actual bufs.
indutny pushed a commit to libuv/libuv that referenced this issue Jan 4, 2016
uv__fs_buf_iter currently sets req->bufs to NULL after it is done, but
if the operation fails with EINTR then it will be retried, at which
point it expects the bufs to not be NULL, causing a seg fault as in
nodejs/node#4291.

uv__fs_buf_iter should not set req->bufs to NULL if the operation
fails with EINTR.

Also, when it sets req->bufs to NULL, it should set req->nbufs to 0 as
well, so we don't have the messy situation of a positive nbufs with no
actual bufs.

PR-URL: #661
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@indutny
Copy link
Member

indutny commented Jan 19, 2016

I believe this should be fixed by libuv update. cc @saghul ;)

@saghul
Copy link
Member

saghul commented Jan 19, 2016

Yeah, the next libuv upgrade includes a fix for this.

@MylesBorins
Copy link
Contributor

@saghul @nodejs/lts should we backport the patch to LTS?

@indutny
Copy link
Member

indutny commented Jan 19, 2016

I believe we should.

@rvagg
Copy link
Member

rvagg commented Jan 20, 2016

I'll reserve final judgement until I see the libuv delta, if it's low-impact then that ought to be fine

@saghul
Copy link
Member

saghul commented Jan 20, 2016

Agreed with @rvagg. Also, since we keep backwards API compatibility and a stable ABI, all libuv upgrades will end up on LTS, right?

@rvagg
Copy link
Member

rvagg commented Jan 20, 2016

I think we had this discussion not long ago, didn't we? I'd lean toward not making a firm rule about it but in general ABI and API compat upgrades in libuv would end up in LTS. If there was a particularly big delta then that may be cause for concern.

saghul added a commit to saghul/node that referenced this issue Apr 7, 2016
saghul added a commit that referenced this issue Apr 7, 2016
Fixes: #5737
Fixes: #4643
Fixes: #4291
Fixes: nodejs/node-v0.x-archive#8960
Refs: #3594
PR-URL: #5994
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
MylesBorins pushed a commit that referenced this issue Apr 19, 2016
Fixes: #5737
Fixes: #4643
Fixes: #4291
Fixes: nodejs/node-v0.x-archive#8960
Refs: #3594
PR-URL: #5994
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
saghul added a commit to saghul/node that referenced this issue Jul 11, 2016
Fixes: nodejs#5737
Fixes: nodejs#4643
Fixes: nodejs#4291
Fixes: nodejs/node-v0.x-archive#8960
Refs: nodejs#3594
PR-URL: nodejs#5994
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
MylesBorins pushed a commit that referenced this issue Jul 11, 2016
Fixes: #5737
Fixes: #4643
Fixes: #4291
Fixes: nodejs/node-v0.x-archive#8960
Refs: #3594
PR-URL: #5994
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
MylesBorins pushed a commit that referenced this issue Jul 11, 2016
Fixes: #5737
Fixes: #4643
Fixes: #4291
Fixes: nodejs/node-v0.x-archive#8960
Refs: #3594
PR-URL: #5994
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
MylesBorins pushed a commit that referenced this issue Jul 12, 2016
Fixes: #5737
Fixes: #4643
Fixes: #4291
Fixes: nodejs/node-v0.x-archive#8960
Refs: #3594
PR-URL: #5994
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
MylesBorins pushed a commit that referenced this issue Jul 14, 2016
Fixes: #5737
Fixes: #4643
Fixes: #4291
Fixes: nodejs/node-v0.x-archive#8960
Refs: #3594
PR-URL: #5994
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
MylesBorins pushed a commit that referenced this issue Jul 14, 2016
Fixes: #5737
Fixes: #4643
Fixes: #4291
Fixes: nodejs/node-v0.x-archive#8960
Refs: #3594
PR-URL: #5994
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. libuv Issues and PRs related to the libuv dependency or the uv binding.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants