From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: tarantool-patches@dev.tarantool.org, kyukhin@tarantool.org Subject: [Tarantool-patches] [PATCH 01/15] fio: don't use shared buffer in pread() Date: Wed, 24 Mar 2021 22:24:21 +0100 [thread overview] Message-ID: <81b10cd4d2d6b9657dc4af7ac92ff3e6fcd882ea.1616620860.git.v.shpilevoy@tarantool.org> (raw) In-Reply-To: <cover.1616620860.git.v.shpilevoy@tarantool.org> fio:pread() used buffer.IBUF_SHARED, which might be reused after a yield. As a result, if pread() was called from 2 different fibers or in parallel with something else using IBUF_SHARED, it would turn the buffer into garbage for all parallel usages. The same problem existed for read(), and was fixed in c7c24f841322528c17714482d150b769d0afcddb ("fio: Fix race condition in fio.read"). But apparently pread() was missed. What is worse, the original commit's test passed even without the fix from that commit. Because it didn't check the results of read()s called from 2 fibers. The patch fixes pread() and adds a test covering both read() and pread(). The old test from the original commit is dropped. Follow up #3187 (cherry picked from commit 24d86294b693867afd02a8847649026ce6b6fb4f) --- src/lua/fio.lua | 3 +- test/app/fio.result | 103 ++++++++++++++++++++++++++---------------- test/app/fio.test.lua | 68 ++++++++++++++++------------ 3 files changed, 103 insertions(+), 71 deletions(-) diff --git a/src/lua/fio.lua b/src/lua/fio.lua index 748efed14..a47bf2192 100644 --- a/src/lua/fio.lua +++ b/src/lua/fio.lua @@ -95,8 +95,7 @@ fio_methods.pread = function(self, buf, size, offset) if not ffi.istype(const_char_ptr_t, buf) then offset = size size = buf - tmpbuf = buffer.IBUF_SHARED - tmpbuf:reset() + tmpbuf = buffer.ibuf() buf = tmpbuf:reserve(size) end local res, err = internal.pread(self.fh, buf, size, offset) diff --git a/test/app/fio.result b/test/app/fio.result index f5820c199..03134b6af 100644 --- a/test/app/fio.result +++ b/test/app/fio.result @@ -7,6 +7,9 @@ ffi = require 'ffi' buffer = require 'buffer' --- ... +fiber = require('fiber') +--- +... test_run = require('test_run').new() --- ... @@ -1179,90 +1182,110 @@ fio.unlink(tmpfile) --- - true ... -tmp1 = fio.pathjoin(tmpdir, "tmp1") +fio.rmdir(tmpdir) --- +- true ... -tmp2= fio.pathjoin(tmpdir, "tmp2") +-- +-- gh-4439: mktree error handling fix - creation of existing file/directory +-- +fio.mktree('/dev/null') --- +- false +- 'Error creating directory /dev/null: File exists' ... -test_run:cmd("setopt delimiter ';'") +fio.mktree('/dev/null/dir') --- -- true +- false +- 'Error creating directory /dev/null: File exists' ... -function write_file(name, odd) - local fh = fio.open(name, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, 0777) - if odd then - fh:write(string.rep('1', 100)) - else - fh:write(string.rep('2', 100)) - end - fh:write(name) - fh:seek(0) - return fh -end; +-- +-- read() and pread() should not use a shared buffer so as not to clash with +-- other fibers during yield. +-- +rights = tonumber('0777', 8) --- ... -test_run:cmd("setopt delimiter ''"); +flags = {'O_RDWR', 'O_TRUNC', 'O_CREAT'} --- -- true ... -fh1 = write_file(tmp1) +tmpdir = fio.tempdir() --- ... -fh2 = write_file(tmp2) +file1 = fio.pathjoin(tmpdir, 'file1') --- ... -fiber = require('fiber') +file2 = fio.pathjoin(tmpdir, 'file2') --- ... -digest = require('digest') +fd1 = fio.open(file1, flags, rights) --- ... -str = fh1:read() +fd2 = fio.open(file2, flags, rights) --- ... -fh1:seek(0) +fd1:write('1'), fd1:seek(0) --- +- true - 0 ... -hash = digest.crc32(str) +fd2:write('2'), fd2:seek(0) --- +- true +- 0 ... -ch = fiber.channel(1) +res1, res2 = nil --- ... -f1 = fiber.create(function() str = fh1:read() ch:put(digest.crc32(str)) end) +do \ + fiber.create(function() res1 = fd1:read() end) \ + fiber.create(function() res2 = fd2:read() end) \ +end --- ... -f2 = fiber.create(function() str = fh2:read() end) +_ = test_run:wait_cond(function() return res1 and res2 end) --- ... -ch:get() == hash +assert(res1 == '1') --- - true ... -fio.unlink(tmp1) +assert(res2 == '2') --- - true ... -fio.unlink(tmp2) +-- +-- The same with pread(). +-- +res1, res2 = nil +--- +... +do \ + fiber.create(function() res1 = fd1:pread(1, 0) end) \ + fiber.create(function() res2 = fd2:pread(1, 0) end) \ +end +--- +... +_ = test_run:wait_cond(function() return res1 and res2 end) +--- +... +assert(res1 == '1') --- - true ... -fio.rmdir(tmpdir) +assert(res2 == '2') --- - true ... --- --- gh-4439: mktree error handling fix - creation of existing file/directory --- -fio.mktree('/dev/null') +fd1:close() --- -- false -- 'Error creating directory /dev/null: File exists' +- true ... -fio.mktree('/dev/null/dir') +fd2:close() --- -- false -- 'Error creating directory /dev/null: File exists' +- true +... +fio.rmtree(tmpdir) +--- +- true ... diff --git a/test/app/fio.test.lua b/test/app/fio.test.lua index b2c53e3e3..4e0a2e87f 100644 --- a/test/app/fio.test.lua +++ b/test/app/fio.test.lua @@ -1,6 +1,7 @@ fio = require 'fio' ffi = require 'ffi' buffer = require 'buffer' +fiber = require('fiber') test_run = require('test_run').new() -- umask @@ -381,38 +382,47 @@ fio.path.lexists(link) fio.unlink(link) fio.unlink(tmpfile) -tmp1 = fio.pathjoin(tmpdir, "tmp1") -tmp2= fio.pathjoin(tmpdir, "tmp2") -test_run:cmd("setopt delimiter ';'") -function write_file(name, odd) - local fh = fio.open(name, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, 0777) - if odd then - fh:write(string.rep('1', 100)) - else - fh:write(string.rep('2', 100)) - end - fh:write(name) - fh:seek(0) - return fh -end; -test_run:cmd("setopt delimiter ''"); -fh1 = write_file(tmp1) -fh2 = write_file(tmp2) -fiber = require('fiber') -digest = require('digest') -str = fh1:read() -fh1:seek(0) -hash = digest.crc32(str) -ch = fiber.channel(1) -f1 = fiber.create(function() str = fh1:read() ch:put(digest.crc32(str)) end) -f2 = fiber.create(function() str = fh2:read() end) -ch:get() == hash - -fio.unlink(tmp1) -fio.unlink(tmp2) fio.rmdir(tmpdir) -- -- gh-4439: mktree error handling fix - creation of existing file/directory -- fio.mktree('/dev/null') fio.mktree('/dev/null/dir') + +-- +-- read() and pread() should not use a shared buffer so as not to clash with +-- other fibers during yield. +-- +rights = tonumber('0777', 8) +flags = {'O_RDWR', 'O_TRUNC', 'O_CREAT'} +tmpdir = fio.tempdir() +file1 = fio.pathjoin(tmpdir, 'file1') +file2 = fio.pathjoin(tmpdir, 'file2') +fd1 = fio.open(file1, flags, rights) +fd2 = fio.open(file2, flags, rights) +fd1:write('1'), fd1:seek(0) +fd2:write('2'), fd2:seek(0) + +res1, res2 = nil +do \ + fiber.create(function() res1 = fd1:read() end) \ + fiber.create(function() res2 = fd2:read() end) \ +end +_ = test_run:wait_cond(function() return res1 and res2 end) +assert(res1 == '1') +assert(res2 == '2') +-- +-- The same with pread(). +-- +res1, res2 = nil +do \ + fiber.create(function() res1 = fd1:pread(1, 0) end) \ + fiber.create(function() res2 = fd2:pread(1, 0) end) \ +end +_ = test_run:wait_cond(function() return res1 and res2 end) +assert(res1 == '1') +assert(res2 == '2') + +fd1:close() +fd2:close() +fio.rmtree(tmpdir) -- 2.24.3 (Apple Git-128)
next prev parent reply other threads:[~2021-03-24 21:25 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-24 21:24 [Tarantool-patches] [PATCH 00/15] Cord buffer, static alloc, and Lua GC bug for 1.10 Vladislav Shpilevoy via Tarantool-patches 2021-03-24 21:24 ` Vladislav Shpilevoy via Tarantool-patches [this message] 2021-03-24 21:24 ` [Tarantool-patches] [PATCH 10/15] uri: replace static_alloc with ffi stash and ibuf Vladislav Shpilevoy via Tarantool-patches 2021-03-24 21:24 ` [Tarantool-patches] [PATCH 11/15] lua: use lua_pushfstring() instead of tt_sprintf() Vladislav Shpilevoy via Tarantool-patches 2021-03-24 21:24 ` [Tarantool-patches] [PATCH 12/15] sio: rework sio_strfaddr() Vladislav Shpilevoy via Tarantool-patches 2021-03-24 21:24 ` [Tarantool-patches] [PATCH 13/15] sio: increase SERVICE_NAME_MAXLEN size Vladislav Shpilevoy via Tarantool-patches 2021-03-24 21:24 ` [Tarantool-patches] [PATCH 14/15] sio: introduce and use sio_snprintf() Vladislav Shpilevoy via Tarantool-patches 2021-03-24 21:24 ` [Tarantool-patches] [PATCH 15/15] buffer: remove Lua registers Vladislav Shpilevoy via Tarantool-patches 2021-03-24 21:24 ` [Tarantool-patches] [PATCH 02/15] test: don't use IBUF_SHARED in the tests Vladislav Shpilevoy via Tarantool-patches 2021-03-24 21:24 ` [Tarantool-patches] [PATCH 03/15] tuple: pass global ibuf explicitly where possible Vladislav Shpilevoy via Tarantool-patches 2021-03-24 21:24 ` [Tarantool-patches] [PATCH 04/15] iconv: take errno before reseting the context Vladislav Shpilevoy via Tarantool-patches 2021-03-24 21:24 ` [Tarantool-patches] [PATCH 05/15] cord_buf: introduce cord_buf API Vladislav Shpilevoy via Tarantool-patches 2021-03-24 21:24 ` [Tarantool-patches] [PATCH 06/15] cord_buf: introduce ownership management Vladislav Shpilevoy via Tarantool-patches 2021-03-24 21:24 ` [Tarantool-patches] [PATCH 07/15] buffer: implement ffi stash Vladislav Shpilevoy via Tarantool-patches 2021-03-24 21:24 ` [Tarantool-patches] [PATCH 08/15] uuid: replace static_alloc with " Vladislav Shpilevoy via Tarantool-patches 2021-03-24 21:24 ` [Tarantool-patches] [PATCH 09/15] uuid: drop tt_uuid_str() from Lua Vladislav Shpilevoy via Tarantool-patches 2021-03-29 15:41 ` [Tarantool-patches] [PATCH 00/15] Cord buffer, static alloc, and Lua GC bug for 1.10 Kirill Yukhin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=81b10cd4d2d6b9657dc4af7ac92ff3e6fcd882ea.1616620860.git.v.shpilevoy@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=kyukhin@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 01/15] fio: don'\''t use shared buffer in pread()' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox