Heap-use-after-free in FS dir API #384
Comments
|
|
|
I think this should be fixed #385, If you can please try it. |
|
I pulled revision cb57fbc from zhaozg/luv, but unfortunately, the same crash happened. The back trace was the same as originally reported. |
|
I found then reason https://github.com/libuv/libuv/blob/v1.x/test/test-fs-readdir.c#L289-L319, luv not do it in right logic. |
|
The PR #385 update, please again, thanks. local uv = nil
local p = p
if p then
uv = require'uv'
else
uv = require'luv'
local inspect = require'inspect'
local unpack = unpack or table.unpack
p = function(...)
local r = {}
for k,v in pairs({...}) do
r[k] = type(v)=='table' and inspect(v) or v
end
print(unpack(r))
end
end
-- use object in sync mode
---[[
do
local dir,cnt = assert(uv.fs_opendir('.')),0
local dirs = dir:readdir()
while dirs do
cnt=cnt+1
p(cnt, dirs)
dirs = dir:readdir()
end
assert(dir:closedir()==true)
print(dir, 'closed', 'total', cnt)
end
uv.run()
collectgarbage()
--]]
-- use object in async mode
---[[
do
local cnt = 0
local function opendir_cb(errx, dir)
assert(not errx, errx)
local function readdir_cb(err, dirs)
assert(not err)
if dirs then
cnt=cnt+1
p(cnt, dirs)
assert(dir:readdir(readdir_cb))
else
assert(dir:closedir(function(erry, result)
assert(not erry, erry)
assert(result)
print(dir, 'closed', 'total', cnt)
end))
end
end
dir:readdir(readdir_cb)
end
assert(uv.fs_opendir('.', opendir_cb))
uv.run()
collectgarbage()
end
--]]
-- use bind api in sync mode
---[[
do
local dir,cnt = assert(uv.fs_opendir('.')),0
local dirs = uv.fs_readdir(dir)
while dirs do
cnt=cnt+1
p(cnt, dirs)
dirs = uv.fs_readdir(dir)
end
assert(uv.fs_closedir(dir)==true)
print(dir, 'closed', 'total', cnt)
end
uv.run()
collectgarbage()
--]]
-- use bind in async mode
---[[
do
local cnt = 0
local function opendir_cb(errx, dir)
assert(not errx, errx)
local function readdir_cb(err, dirs)
assert(not err)
if dirs then
cnt=cnt+1
p(cnt, dirs)
assert(uv.fs_readdir(dir,readdir_cb))
else
assert(uv.fs_closedir(dir, function(erry, result)
assert(not erry, erry)
assert(result)
print(dir, 'closed', 'total', cnt)
end))
end
end
uv.fs_readdir(dir,readdir_cb)
end
assert(uv.fs_opendir('.', opendir_cb))
uv.run()
collectgarbage()
end
--]]
-- auto dir auto closed gc
---[[
do
local dir = assert(uv.fs_opendir('.'))
dir = nil
uv.run()
collectgarbage()
end
--]] |
|
Unfortunately, I pulled b8d2233 but the same issue happened. I double checked the source code and everything was there from your latest patch. To make sure, I recompiled everything twice, but got the same crash. For the async dir test, if I comment out the uv.fs_readdir() inside the readdir_cb, I got the same output as the sync test. All items under the current folder were listed. So what's the purpose of the fs_readdir() inside the readdir_cb()? |
|
are you use pure luv, not luvit?
to handle more / a lots files in dir |
|
Created a standalone test from the async dir: local uv = require'luv'
local function opendir_cb(err, dir)
assert(not err)
local function readdir_cb(err, dirs)
assert(not err)
if dirs then
uv.fs_readdir(dir, readdir_cb)
else
--uv.fs_closedir(dir)
end
end
uv.fs_readdir(dir, readdir_cb)
end
uv.fs_opendir('.', opendir_cb, 50)
uv.run()As soon as the uv.fs_closedir() is uncommented, crash would happen. Without closedir(), everything is fine. No crash, no leak. |
|
change uv.fs_opendir('.', opendir_cb, 50) to uv.fs_opendir('.', opendir_cb), and try |
It didn't change the behavior. As long as uv.fs_closedir(dir) is in place, it would crash. |
|
Here's the valgrind output I'm getting: I think what's happening is this:
Will look into it more later. Probably worth looking at Libuv's EDIT: Worth noting that the nested local uv = require'luv'
local function opendir_cb(err, dir)
assert(not err)
local function readdir_cb(err, dirs)
assert(not err)
uv.fs_closedir(dir)
end
uv.fs_readdir(dir, readdir_cb)
end
uv.fs_opendir('.', opendir_cb)
uv.run() |
|
@squeek502 good point |
|
Seems to be a race condition. Added some debug printing and got these outputs from two different runs of the script in my last comment: No invalid read here though, same code: This race condition also affects the Libuv tests but for some reason Valgrind isn't detecting the use-after-free there: That last Will do some more investigating in the next few days. |
|
The following definition is not correct in defining the local scope: The local scope function needs to be predeclared; otherwise the function escapes to the global scope: It's just how Lua works. http://lua-users.org/wiki/MinimisingClosures |
|
@rphillips interesting, but I don't think that affects the use-after-free/race condition here. |
|
Confirmed that this is a race condition that exists in Libuv. The Libuv test runner uses a separate process for each test, so using Valgrind on the test runner process wasn't detecting the use-after-free in the child process. Was able to reproduce it in the Libuv tests via Will write up an issue in the Libuv issue tracker for this. EDIT: Reported here: libuv/libuv#2496 |
|
@squeek502 I think that is not same, https://github.com/libuv/libuv/blob/v1.x/test/test-fs-readdir.c#L290-L295 show uv_fs_req_cleanup(readdir_req) before uv_fs_closedir(closedir_req), but luv do uv_fs_closedir(closedir_req) before uv_fs_req_cleanup(readdir_req). That means a new pattern to handle this. |
|
@zhaozg You're right, the fix for that Libuv test was just to move the |
|
Replacing Lines 376 to 377 in cf89aea with if (req->fs_type == UV_FS_SCANDIR) {
luv_fulfill_req(L, (luv_req_t*)req->data, nargs);
}
else {
// cleanup the uv_fs_t before the callback is called to avoid
// a race condition when fs_close is called from within
// a fs_readdir callback, see https://github.com/luvit/luv/issues/384
luv_req_t* luv_req = req->data;
uv_fs_req_cleanup(req);
req->data = NULL;
luv_fulfill_req(L, luv_req, nargs);
luv_cleanup_req(L, luv_req);
}should fix it. |
|
please review https://github.com/luvit/luv/pull/385/files. |
|
Confirmed. The crash was gone after update to f6b41f5. |
This was found when running test-fs.lua: "fs.{open,read,close}dir with more entry".
The uv.fs_readdir(dir, readdir_cb) inside the readdir_cb callback would trigger the crash. Looks like the current FS directory API does not support nested calls.
=================================================================
==7149==ERROR: AddressSanitizer: heap-use-after-free on address 0x606000003fe0 at pc 0x7f0c83ab04d0 bp 0x7fff181900a0 sp 0x7fff18190098
READ of size 8 at 0x606000003fe0 thread T0
#0 0x7f0c83ab04cf in uv__fs_readdir_cleanup /home/lua-projects/libuv/src/uv-common.c:689:18
#1 0x7f0c83ac9300 in uv_fs_req_cleanup /home/lua-projects/libuv/src/unix/fs.c:1870:5
#2 0x7f0c83ab14c3 in uv__work_done /home/lua-projects/libuv/src/threadpool.c:313:5
#3 0x7f0c83ab8c6e in uv__async_io /home/lua-projects/libuv/src/unix/async.c:147:5
#4 0x7f0c83accecc in uv__io_poll /home/lua-projects/libuv/src/unix/linux-core.c:384:11
#5 0x7f0c83ab9a37 in uv_run /home/lua-projects/libuv/src/unix/core.c:373:5
#6 0x7f0c83a849ec in luv_run /home/lua-projects/lua-modules/luv/src/loop.c:34:13
#7 0x5cb016 in lj_BC_FUNCC /home/lua-projects/luajit/src/lj_vm.S:809
0x606000003fe0 is located 0 bytes inside of 56-byte region [0x606000003fe0,0x606000004018)
freed by thread T0 here:
#0 0x4e7928 in __interceptor_free /home/llvm/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:124
#1 0x7f0c83aac542 in uv__free /home/lua-projects/libuv/src/uv-common.c:88:3
#2 0x7f0c83ac3165 in uv__fs_closedir /home/lua-projects/libuv/src/unix/fs.c:524:3
#3 0x7f0c83ac3165 in uv__fs_work /home/lua-projects/libuv/src/unix/fs.c:1423
#4 0x7f0c83ac7539 in uv_fs_closedir /home/lua-projects/libuv/src/unix/fs.c:1727:3
#5 0x7f0c83a9d56b in luv_fs_closedir /home/lua-projects/lua-modules/luv/src/fs.c:819:3
#6 0x5cb016 in lj_BC_FUNCC /home/lua-projects/luajit/src/lj_vm.S:809
previously allocated by thread T4 here:
#0 0x4e7d07 in malloc /home/llvm/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:146
#1 0x7f0c83ac154d in uv__fs_opendir /home/lua-projects/libuv/src/unix/fs.c:451:9
#2 0x7f0c83ac154d in uv__fs_work /home/lua-projects/libuv/src/unix/fs.c:1421
#3 0x7f0c83ab1ee1 in worker /home/lua-projects/libuv/src/threadpool.c:122:5
#4 0x7f0c8781e6da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
Thread T4 created by T0 here:
#0 0x434100 in __interceptor_pthread_create /home/llvm/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:210
#1 0x7f0c83ae9ceb in uv_thread_create_ex /home/lua-projects/libuv/src/unix/thread.c:258:9
#2 0x7f0c83ae9a67 in uv_thread_create /home/lua-projects/libuv/src/unix/thread.c:212:10
#3 0x7f0c83ab1144 in init_threads /home/lua-projects/libuv/src/threadpool.c:225:9
#4 0x7f0c83ab1144 in init_once /home/lua-projects/libuv/src/threadpool.c:252
#5 0x7f0c87826826 in __pthread_once_slow (/lib/x86_64-linux-gnu/libpthread.so.0+0xf826)
SUMMARY: AddressSanitizer: heap-use-after-free /home/lua-projects/libuv/src/uv-common.c:689:18 in uv__fs_readdir_cleanup
Shadow bytes around the buggy address:
0x0c0c7fff87a0: fd fd fd fd fa fa fa fa fd fd fd fd fd fd fd fd
0x0c0c7fff87b0: fa fa fa fa fd fd fd fd fd fd fd fd fa fa fa fa
0x0c0c7fff87c0: fd fd fd fd fd fd fd fd fa fa fa fa fd fd fd fd
0x0c0c7fff87d0: fd fd fd fd fa fa fa fa fd fd fd fd fd fd fd fd
0x0c0c7fff87e0: fa fa fa fa fd fd fd fd fd fd fd fd fa fa fa fa
=>0x0c0c7fff87f0: fd fd fd fd fd fd fd fd fa fa fa fa[fd]fd fd fd
0x0c0c7fff8800: fd fd fd fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c0c7fff8810: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c0c7fff8820: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c0c7fff8830: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c0c7fff8840: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
Shadow gap: cc
==7149==ABORTING
The text was updated successfully, but these errors were encountered: