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

Potential uvwasi_fd_table_renumber() deadlock #89

Closed
cjihrig opened this issue Jan 21, 2020 · 0 comments · Fixed by #90
Closed

Potential uvwasi_fd_table_renumber() deadlock #89

cjihrig opened this issue Jan 21, 2020 · 0 comments · Fixed by #90

Comments

@cjihrig
Copy link
Collaborator

cjihrig commented Jan 21, 2020

Refs: nodejs/node#31432 (comment)

uvwasi_fd_table_renumber() locks the file descriptor table, acquires the source and destination mutexes, performs the renumber operation, releases the mutexes, and unlocks the file descriptor table.

An operation that takes two file descriptors (such as uvwasi_path_rename()) could have acquired one of its fd mutex locks, but not the other. If uvwasi_fd_table_renumber() is called during this window of time, deadlock can occur (uvwasi_fd_table_renumber() waits for the mutex that uvwasi_path_rename() has, and uvwasi_path_rename() waits for the file descriptor table lock that uvwasi_fd_table_renumber() has).

The file descriptor table API could add lock and unlock APIs so that calls like uvwasi_path_rename() can acquire everything at once.
cjihrig added a commit that referenced this issue Jan 22, 2020
@cjihrig
prevent locking fd table while holding a mutex
611f84f 
uvwasi_path_rename(), uvwasi_path_link(), uvwasi_path_open(), and
uvwasi_fd_renumber() operate on multiple file descriptors.
uvwasi_fd_renumber() has been updated prior to this commit, and
is not relevant here. The other three functions would perform
the following locking operations:

- lock the file table
- acquire a file descriptor mutex
- unlock the file table
- unlock the file table again
- acquire another file descriptor mutex
- unlock the file table
- unlock the two mutexes

Attempting to acquire the second mutex introduced the possibility
of deadlock because another thread could attempt to acquire the first
mutex while holding the file table lock.

This commit ensures that multiple mutexes are either:
- acquired in a single lock of the file table
- or, only acquired after releasing previously held mutexes

Fixes: #89
@cjihrig cjihrig mentioned this issue Jan 22, 2020
Merged
cjihrig added a commit that referenced this issue Jan 22, 2020
@cjihrig
prevent locking fd table while holding a mutex
c3bef8e 
uvwasi_path_rename(), uvwasi_path_link(), uvwasi_path_open(), and
uvwasi_fd_renumber() operate on multiple file descriptors.
uvwasi_fd_renumber() has been updated prior to this commit, and
is not relevant here. The other three functions would perform
the following locking operations:

- lock the file table
- acquire a file descriptor mutex
- unlock the file table
- unlock the file table again
- acquire another file descriptor mutex
- unlock the file table
- unlock the two mutexes

Attempting to acquire the second mutex introduced the possibility
of deadlock because another thread could attempt to acquire the first
mutex while holding the file table lock.

This commit ensures that multiple mutexes are either:
- acquired in a single lock of the file table
- or, only acquired after releasing previously held mutexes

Fixes: #89
cjihrig added a commit to cjihrig/node that referenced this issue Jan 23, 2020
@cjihrig
deps: uvwasi: cherry-pick c3bef8e
0d0c835 
Original commit message:
    prevent locking fd table while holding a mutex

    uvwasi_path_rename(), uvwasi_path_link(),
    uvwasi_path_open(), and uvwasi_fd_renumber() operate
    on multiple file descriptors. uvwasi_fd_renumber() has
    been updated prior to this commit, and is not relevant
    here. The other three functions would perform the
    following locking operations:

    - lock the file table
    - acquire a file descriptor mutex
    - unlock the file table
    - unlock the file table again
    - acquire another file descriptor mutex
    - unlock the file table
    - unlock the two mutexes

    Attempting to acquire the second mutex introduced
    the possibility of deadlock because another thread
    could attempt to acquire the first mutex while
    holding the file table lock.

    This commit ensures that multiple mutexes are either:
    - acquired in a single lock of the file table
    - or, only acquired after releasing previously held mutexes

    Fixes: nodejs/uvwasi#89
Trott pushed a commit to Trott/io.js that referenced this issue Jan 23, 2020
@cjihrig @Trott
deps: uvwasi: cherry-pick c3bef8e
c150f9d 
Original commit message:
    prevent locking fd table while holding a mutex

    uvwasi_path_rename(), uvwasi_path_link(),
    uvwasi_path_open(), and uvwasi_fd_renumber() operate
    on multiple file descriptors. uvwasi_fd_renumber() has
    been updated prior to this commit, and is not relevant
    here. The other three functions would perform the
    following locking operations:

    - lock the file table
    - acquire a file descriptor mutex
    - unlock the file table
    - unlock the file table again
    - acquire another file descriptor mutex
    - unlock the file table
    - unlock the two mutexes

    Attempting to acquire the second mutex introduced
    the possibility of deadlock because another thread
    could attempt to acquire the first mutex while
    holding the file table lock.

    This commit ensures that multiple mutexes are either:
    - acquired in a single lock of the file table
    - or, only acquired after releasing previously held mutexes

    Fixes: nodejs/uvwasi#89

PR-URL: nodejs#31432
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
codebytere pushed a commit to nodejs/node that referenced this issue Feb 17, 2020
@cjihrig @codebytere
deps: uvwasi: cherry-pick c3bef8e
2fe0ed3 
Original commit message:
    prevent locking fd table while holding a mutex

    uvwasi_path_rename(), uvwasi_path_link(),
    uvwasi_path_open(), and uvwasi_fd_renumber() operate
    on multiple file descriptors. uvwasi_fd_renumber() has
    been updated prior to this commit, and is not relevant
    here. The other three functions would perform the
    following locking operations:

    - lock the file table
    - acquire a file descriptor mutex
    - unlock the file table
    - unlock the file table again
    - acquire another file descriptor mutex
    - unlock the file table
    - unlock the two mutexes

    Attempting to acquire the second mutex introduced
    the possibility of deadlock because another thread
    could attempt to acquire the first mutex while
    holding the file table lock.

    This commit ensures that multiple mutexes are either:
    - acquired in a single lock of the file table
    - or, only acquired after releasing previously held mutexes

    Fixes: nodejs/uvwasi#89

PR-URL: #31432
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
codebytere pushed a commit to nodejs/node that referenced this issue Mar 15, 2020
@cjihrig @codebytere
deps: uvwasi: cherry-pick c3bef8e
59c7df6 
Original commit message:
    prevent locking fd table while holding a mutex

    uvwasi_path_rename(), uvwasi_path_link(),
    uvwasi_path_open(), and uvwasi_fd_renumber() operate
    on multiple file descriptors. uvwasi_fd_renumber() has
    been updated prior to this commit, and is not relevant
    here. The other three functions would perform the
    following locking operations:

    - lock the file table
    - acquire a file descriptor mutex
    - unlock the file table
    - unlock the file table again
    - acquire another file descriptor mutex
    - unlock the file table
    - unlock the two mutexes

    Attempting to acquire the second mutex introduced
    the possibility of deadlock because another thread
    could attempt to acquire the first mutex while
    holding the file table lock.

    This commit ensures that multiple mutexes are either:
    - acquired in a single lock of the file table
    - or, only acquired after releasing previously held mutexes

    Fixes: nodejs/uvwasi#89

PR-URL: #31432
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
codebytere pushed a commit to nodejs/node that referenced this issue Mar 17, 2020
@cjihrig @codebytere
deps: uvwasi: cherry-pick c3bef8e
3bd1c02 
Original commit message:
    prevent locking fd table while holding a mutex

    uvwasi_path_rename(), uvwasi_path_link(),
    uvwasi_path_open(), and uvwasi_fd_renumber() operate
    on multiple file descriptors. uvwasi_fd_renumber() has
    been updated prior to this commit, and is not relevant
    here. The other three functions would perform the
    following locking operations:

    - lock the file table
    - acquire a file descriptor mutex
    - unlock the file table
    - unlock the file table again
    - acquire another file descriptor mutex
    - unlock the file table
    - unlock the two mutexes

    Attempting to acquire the second mutex introduced
    the possibility of deadlock because another thread
    could attempt to acquire the first mutex while
    holding the file table lock.

    This commit ensures that multiple mutexes are either:
    - acquired in a single lock of the file table
    - or, only acquired after releasing previously held mutexes

    Fixes: nodejs/uvwasi#89

PR-URL: #31432
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

prevent locking fd table while holding a mutex nodejs/uvwasi
1 participant
@cjihrig