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

win,tty: fix uv_tty_set_mode race conditions #1054

Closed
wants to merge 3 commits into from

Conversation

bzoz
Copy link
Member

@bzoz bzoz commented Sep 15, 2016

Additional synchronization is needed to ensure that the program cannot modify the screen state while a line read is getting cancelled.

Also, we need to stop any pending reads before calling SetConsoleMode, or a call to ReadConsole could start while the console is still in raw mode.

We also switch to use a semaphore rather than a critical section because in some cases (uv__cancel_read_console) we need take the lock in the main thread and release it in another thread. Using a semaphore ensures that in such scenario the main thread will still block when trying to acquire the lock

Credit: orangemocha - Alexis Campailla orangemocha@nodejs.org

Fixes: nodejs/node#7837

Additional synchronization is needed to ensure that the program
cannot modify the screen state while a line read is getting cancelled.

Also, we need to stop any pending reads *before* calling SetConsoleMode,
or a call to ReadConsole could start while the console is still in raw
mode.

Credit: orangemocha - Alexis Campailla <orangemocha@nodejs.org>

Fixes: nodejs/node#7837
Copy link
Member

@saghul saghul left a comment

Choose a reason for hiding this comment

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

Left some comments. Can a test for this be written?

/* thread and release it in another thread. Using a semaphore ensures that */
/* in such scenario the main thread will still block when trying to acquire */
/* the lock. */
static HANDLE uv_tty_output_lock;
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a uv_sem_t here instead?

if (InterlockedOr(&uv__restore_screen_state, 0)) {
HANDLE active_screen_buffer = CreateFileA("conout$",
if (status == TRAP_REQUESTED) {
/* If we canceled the read by sending a VK_RETURN event, restore the */
Copy link
Member

Choose a reason for hiding this comment

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

Please use a multi-line comment here, like it was before.

Copy link
Member Author

Choose a reason for hiding this comment

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

What style to use? This:

/* comment comment
 * comment comment
 */

or this:

/* comment comment
   comment comment */

?

@@ -1035,11 +1052,16 @@ static int uv__cancel_read_console(uv_tty_t* handle) {

assert(!(handle->flags & UV_HANDLE_CANCELLATION_PENDING));

/* Hold the output lock during the cancellation, to ensure that further
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@bzoz
Copy link
Member Author

bzoz commented Sep 16, 2016

Updated, PTAL

@@ -111,7 +111,12 @@ static int uv_tty_virtual_offset = -1;
static int uv_tty_virtual_height = -1;
static int uv_tty_virtual_width = -1;

static CRITICAL_SECTION uv_tty_output_lock;
/* We use a semaphore rather than a mutex or critical section because in */
Copy link
Member

Choose a reason for hiding this comment

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

style: can you adjust the comment style as below?

Copy link
Member

@saghul saghul left a comment

Choose a reason for hiding this comment

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

LGTM with a style nit. A test would be nice though, not sure how possible that is. @bzoz?

@bzoz
Copy link
Member Author

bzoz commented Sep 16, 2016

Updated the comment style. I don't think that a test for this is possible, at least I don't know how to make one.

@saghul
Copy link
Member

saghul commented Sep 17, 2016

LGTM with a leap of faith ;-)

saghul pushed a commit that referenced this pull request Sep 18, 2016
Additional synchronization is needed to ensure that the program
cannot modify the screen state while a line read is getting cancelled.

Also, we need to stop any pending reads *before* calling SetConsoleMode,
or a call to ReadConsole could start while the console is still in raw
mode.

Credit: @orangemocha - Alexis Campailla <orangemocha@nodejs.org>
Fixes: nodejs/node#7837
PR-URL: #1054
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
@saghul
Copy link
Member

saghul commented Sep 18, 2016

Cheers, Bartosz, landed in 8414403.

@saghul saghul closed this Sep 18, 2016
addaleax added a commit to addaleax/node that referenced this pull request Oct 27, 2016
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: nodejs#7837
addaleax added a commit to nodejs/node that referenced this pull request Nov 16, 2016
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: #7837
PR-URL: #8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: nodejs#7837
PR-URL: nodejs#8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Jan 23, 2017
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: #7837
PR-URL: #8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Jan 24, 2017
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: #7837
PR-URL: #8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 24, 2017
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: nodejs#7837
PR-URL: nodejs#8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: nodejs#7837
PR-URL: nodejs#8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Jan 31, 2017
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: #7837
PR-URL: #8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this pull request May 16, 2017
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: #7837
PR-URL: #8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this pull request May 18, 2017
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: #7837
PR-URL: #8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: nodejs/node#7837
PR-URL: nodejs/node#8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants