Skip to content

Commit c3bef8e

Browse files
cjihrigcjihrig
committed
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: #89
1 parent ea73af5 commit c3bef8e

File tree

3 files changed

+113
-69
lines changed

3 files changed

+113
-69
lines changed

‎include/fd_table.h

+8-1
Original file line numberDiff line numberDiff line change
@@ -46,17 +46,24 @@ uvwasi_errno_t uvwasi_fd_table_insert_preopen(struct uvwasi_s* uvwasi,
4646
const uv_file fd,
4747
const char* path,
4848
const char* real_path);
49-
uvwasi_errno_t uvwasi_fd_table_get(const struct uvwasi_fd_table_t* table,
49+
uvwasi_errno_t uvwasi_fd_table_get(struct uvwasi_fd_table_t* table,
5050
const uvwasi_fd_t id,
5151
struct uvwasi_fd_wrap_t** wrap,
5252
uvwasi_rights_t rights_base,
5353
uvwasi_rights_t rights_inheriting);
54+
uvwasi_errno_t uvwasi_fd_table_get_nolock(struct uvwasi_fd_table_t* table,
55+
const uvwasi_fd_t id,
56+
struct uvwasi_fd_wrap_t** wrap,
57+
uvwasi_rights_t rights_base,
58+
uvwasi_rights_t rights_inheriting);
5459
uvwasi_errno_t uvwasi_fd_table_remove(struct uvwasi_s* uvwasi,
5560
struct uvwasi_fd_table_t* table,
5661
const uvwasi_fd_t id);
5762
uvwasi_errno_t uvwasi_fd_table_renumber(struct uvwasi_s* uvwasi,
5863
struct uvwasi_fd_table_t* table,
5964
const uvwasi_fd_t dst,
6065
const uvwasi_fd_t src);
66+
uvwasi_errno_t uvwasi_fd_table_lock(struct uvwasi_fd_table_t* table);
67+
uvwasi_errno_t uvwasi_fd_table_unlock(struct uvwasi_fd_table_t* table);
6168

6269
#endif /* __UVWASI_FD_TABLE_H__ */

‎src/fd_table.c

+49-18
Original file line numberDiff line numberDiff line change
@@ -252,44 +252,57 @@ uvwasi_errno_t uvwasi_fd_table_insert_preopen(uvwasi_t* uvwasi,
252252
}
253253

254254

255-
uvwasi_errno_t uvwasi_fd_table_get(const struct uvwasi_fd_table_t* table,
255+
uvwasi_errno_t uvwasi_fd_table_get(struct uvwasi_fd_table_t* table,
256256
const uvwasi_fd_t id,
257257
struct uvwasi_fd_wrap_t** wrap,
258258
uvwasi_rights_t rights_base,
259259
uvwasi_rights_t rights_inheriting) {
260-
struct uvwasi_fd_wrap_t* entry;
261260
uvwasi_errno_t err;
262261

263-
if (table == NULL || wrap == NULL)
262+
if (table == NULL)
264263
return UVWASI_EINVAL;
265264

266-
uv_rwlock_rdlock((uv_rwlock_t *)&table->rwlock);
265+
uv_rwlock_wrlock(&table->rwlock);
266+
err = uvwasi_fd_table_get_nolock(table,
267+
id,
268+
wrap,
269+
rights_base,
270+
rights_inheriting);
271+
uv_rwlock_wrunlock(&table->rwlock);
272+
return err;
273+
}
267274

268-
if (id >= table->size) {
269-
err = UVWASI_EBADF;
270-
goto exit;
271-
}
275+
276+
/* uvwasi_fd_table_get_nolock() retrieves a file descriptor and locks its mutex,
277+
but does not lock the file descriptor table like uvwasi_fd_table_get() does.
278+
*/
279+
uvwasi_errno_t uvwasi_fd_table_get_nolock(struct uvwasi_fd_table_t* table,
280+
const uvwasi_fd_t id,
281+
struct uvwasi_fd_wrap_t** wrap,
282+
uvwasi_rights_t rights_base,
283+
uvwasi_rights_t rights_inheriting) {
284+
struct uvwasi_fd_wrap_t* entry;
285+
286+
if (table == NULL || wrap == NULL)
287+
return UVWASI_EINVAL;
288+
289+
if (id >= table->size)
290+
return UVWASI_EBADF;
272291

273292
entry = table->fds[id];
274293

275-
if (entry == NULL || entry->id != id) {
276-
err = UVWASI_EBADF;
277-
goto exit;
278-
}
294+
if (entry == NULL || entry->id != id)
295+
return UVWASI_EBADF;
279296

280297
/* Validate that the fd has the necessary rights. */
281298
if ((~entry->rights_base & rights_base) != 0 ||
282299
(~entry->rights_inheriting & rights_inheriting) != 0) {
283-
err = UVWASI_ENOTCAPABLE;
284-
goto exit;
300+
return UVWASI_ENOTCAPABLE;
285301
}
286302

287303
uv_mutex_lock(&entry->mutex);
288304
*wrap = entry;
289-
err = UVWASI_ESUCCESS;
290-
exit:
291-
uv_rwlock_rdunlock((uv_rwlock_t *)&table->rwlock);
292-
return err;
305+
return UVWASI_ESUCCESS;
293306
}
294307

295308

@@ -389,3 +402,21 @@ uvwasi_errno_t uvwasi_fd_table_renumber(struct uvwasi_s* uvwasi,
389402
uv_rwlock_wrunlock(&table->rwlock);
390403
return err;
391404
}
405+
406+
407+
uvwasi_errno_t uvwasi_fd_table_lock(struct uvwasi_fd_table_t* table) {
408+
if (table == NULL)
409+
return UVWASI_EINVAL;
410+
411+
uv_rwlock_wrlock(&table->rwlock);
412+
return UVWASI_ESUCCESS;
413+
}
414+
415+
416+
uvwasi_errno_t uvwasi_fd_table_unlock(struct uvwasi_fd_table_t* table) {
417+
if (table == NULL)
418+
return UVWASI_EINVAL;
419+
420+
uv_rwlock_wrunlock(&table->rwlock);
421+
return UVWASI_ESUCCESS;
422+
}

‎src/uvwasi.c

+56-50
Original file line numberDiff line numberDiff line change
@@ -1763,37 +1763,41 @@ uvwasi_errno_t uvwasi_path_link(uvwasi_t* uvwasi,
17631763
if (uvwasi == NULL || old_path == NULL || new_path == NULL)
17641764
return UVWASI_EINVAL;
17651765

1766-
if (old_fd == new_fd) {
1767-
err = uvwasi_fd_table_get(&uvwasi->fds,
1768-
old_fd,
1769-
&old_wrap,
1770-
UVWASI_RIGHT_PATH_LINK_SOURCE |
1771-
UVWASI_RIGHT_PATH_LINK_TARGET,
1772-
0);
1773-
if (err != UVWASI_ESUCCESS)
1774-
return err;
1766+
uvwasi_fd_table_lock(&uvwasi->fds);
17751767

1768+
if (old_fd == new_fd) {
1769+
err = uvwasi_fd_table_get_nolock(&uvwasi->fds,
1770+
old_fd,
1771+
&old_wrap,
1772+
UVWASI_RIGHT_PATH_LINK_SOURCE |
1773+
UVWASI_RIGHT_PATH_LINK_TARGET,
1774+
0);
17761775
new_wrap = old_wrap;
17771776
} else {
1778-
err = uvwasi_fd_table_get(&uvwasi->fds,
1779-
old_fd,
1780-
&old_wrap,
1781-
UVWASI_RIGHT_PATH_LINK_SOURCE,
1782-
0);
1783-
if (err != UVWASI_ESUCCESS)
1784-
return err;
1785-
1786-
err = uvwasi_fd_table_get(&uvwasi->fds,
1787-
new_fd,
1788-
&new_wrap,
1789-
UVWASI_RIGHT_PATH_LINK_TARGET,
1790-
0);
1777+
err = uvwasi_fd_table_get_nolock(&uvwasi->fds,
1778+
old_fd,
1779+
&old_wrap,
1780+
UVWASI_RIGHT_PATH_LINK_SOURCE,
1781+
0);
17911782
if (err != UVWASI_ESUCCESS) {
1792-
uv_mutex_unlock(&old_wrap->mutex);
1783+
uvwasi_fd_table_unlock(&uvwasi->fds);
17931784
return err;
17941785
}
1786+
1787+
err = uvwasi_fd_table_get_nolock(&uvwasi->fds,
1788+
new_fd,
1789+
&new_wrap,
1790+
UVWASI_RIGHT_PATH_LINK_TARGET,
1791+
0);
1792+
if (err != UVWASI_ESUCCESS)
1793+
uv_mutex_unlock(&old_wrap->mutex);
17951794
}
17961795

1796+
uvwasi_fd_table_unlock(&uvwasi->fds);
1797+
1798+
if (err != UVWASI_ESUCCESS)
1799+
return err;
1800+
17971801
err = uvwasi__resolve_path(uvwasi,
17981802
old_wrap,
17991803
old_path,
@@ -1922,12 +1926,11 @@ uvwasi_errno_t uvwasi_path_open(uvwasi_t* uvwasi,
19221926
}
19231927

19241928
r = uv_fs_open(NULL, &req, resolved_path, flags, 0666, NULL);
1929+
uv_mutex_unlock(&dirfd_wrap->mutex);
19251930
uv_fs_req_cleanup(&req);
19261931

1927-
if (r < 0) {
1928-
uv_mutex_unlock(&dirfd_wrap->mutex);
1932+
if (r < 0)
19291933
return uvwasi__translate_uv_error(r);
1930-
}
19311934

19321935
/* Not all platforms support UV_FS_O_DIRECTORY, so get the file type and check
19331936
it here. */
@@ -1960,11 +1963,9 @@ uvwasi_errno_t uvwasi_path_open(uvwasi_t* uvwasi,
19601963

19611964
*fd = wrap->id;
19621965
uv_mutex_unlock(&wrap->mutex);
1963-
uv_mutex_unlock(&dirfd_wrap->mutex);
19641966
return UVWASI_ESUCCESS;
19651967

19661968
close_file_and_error_exit:
1967-
uv_mutex_unlock(&dirfd_wrap->mutex);
19681969
uv_fs_close(NULL, &req, r, NULL);
19691970
uv_fs_req_cleanup(&req);
19701971
return err;
@@ -2079,36 +2080,41 @@ uvwasi_errno_t uvwasi_path_rename(uvwasi_t* uvwasi,
20792080
if (uvwasi == NULL || old_path == NULL || new_path == NULL)
20802081
return UVWASI_EINVAL;
20812082

2083+
uvwasi_fd_table_lock(&uvwasi->fds);
2084+
20822085
if (old_fd == new_fd) {
2083-
err = uvwasi_fd_table_get(&uvwasi->fds,
2084-
old_fd,
2085-
&old_wrap,
2086-
UVWASI_RIGHT_PATH_RENAME_SOURCE |
2087-
UVWASI_RIGHT_PATH_RENAME_TARGET,
2088-
0);
2089-
if (err != UVWASI_ESUCCESS)
2090-
return err;
2086+
err = uvwasi_fd_table_get_nolock(&uvwasi->fds,
2087+
old_fd,
2088+
&old_wrap,
2089+
UVWASI_RIGHT_PATH_RENAME_SOURCE |
2090+
UVWASI_RIGHT_PATH_RENAME_TARGET,
2091+
0);
20912092
new_wrap = old_wrap;
20922093
} else {
2093-
err = uvwasi_fd_table_get(&uvwasi->fds,
2094-
old_fd,
2095-
&old_wrap,
2096-
UVWASI_RIGHT_PATH_RENAME_SOURCE,
2097-
0);
2098-
if (err != UVWASI_ESUCCESS)
2099-
return err;
2100-
2101-
err = uvwasi_fd_table_get(&uvwasi->fds,
2102-
new_fd,
2103-
&new_wrap,
2104-
UVWASI_RIGHT_PATH_RENAME_TARGET,
2105-
0);
2094+
err = uvwasi_fd_table_get_nolock(&uvwasi->fds,
2095+
old_fd,
2096+
&old_wrap,
2097+
UVWASI_RIGHT_PATH_RENAME_SOURCE,
2098+
0);
21062099
if (err != UVWASI_ESUCCESS) {
2107-
uv_mutex_unlock(&old_wrap->mutex);
2100+
uvwasi_fd_table_unlock(&uvwasi->fds);
21082101
return err;
21092102
}
2103+
2104+
err = uvwasi_fd_table_get_nolock(&uvwasi->fds,
2105+
new_fd,
2106+
&new_wrap,
2107+
UVWASI_RIGHT_PATH_RENAME_TARGET,
2108+
0);
2109+
if (err != UVWASI_ESUCCESS)
2110+
uv_mutex_unlock(&old_wrap->mutex);
21102111
}
21112112

2113+
uvwasi_fd_table_unlock(&uvwasi->fds);
2114+
2115+
if (err != UVWASI_ESUCCESS)
2116+
return err;
2117+
21122118
err = uvwasi__resolve_path(uvwasi,
21132119
old_wrap,
21142120
old_path,

0 commit comments

Comments
 (0)